refactor: extract TranslationManager to singleton service by xiaocang · Pull Request #2 · xiaocang/easydict_win32

xiaocang · 2026-01-21T09:50:37Z

No description provided.

Copilot

Pull request overview

This pull request refactors the TranslationManager into a singleton service to eliminate code duplication across multiple windows (MiniWindow, MainPage, FixedWindow). The refactoring centralizes HttpClient management, service configuration, and caching in a single shared instance.

Changes:

Created a new TranslationManagerService singleton that manages a shared TranslationManager instance
Removed duplicate TranslationManager instantiation and configuration code from three window classes
Updated all windows to use the shared singleton instance instead of creating their own instances

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
TranslationManagerService.cs	New singleton service that provides shared TranslationManager instance with centralized configuration
MiniWindow.xaml.cs	Removed local TranslationManager field and configuration; now uses shared singleton instance
MainPage.xaml.cs	Removed local TranslationManager field and configuration; now uses shared singleton instance
FixedWindow.xaml.cs	Removed local TranslationManager field and configuration; now uses shared singleton instance
LanguageDetectionService.cs	Changed cache clearing from Dispose to Compact to avoid disposing shared cache

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

dotnet/src/Easydict.WinUI/Services/TranslationManagerService.cs

dotnet/src/Easydict.WinUI/Views/MiniWindow.xaml.cs

dotnet/src/Easydict.WinUI/Views/FixedWindow.xaml.cs

dotnet/src/Easydict.WinUI/Views/MainPage.xaml.cs

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

dotnet/src/Easydict.WinUI/Views/SettingsPage.xaml.cs

dotnet/src/Easydict.WinUI/Services/TranslationManagerService.cs

dotnet/src/Easydict.WinUI/Services/LanguageDetectionService.cs

dotnet/src/Easydict.WinUI/Services/TranslationManagerService.cs

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-21T12:39:22Z

dotnet/src/Easydict.WinUI/Services/LanguageDetectionService.cs

+    /// Gets the TranslationManager on-demand from the singleton service.
+    /// This ensures we always use the current manager instance, even after proxy reconfiguration.
+    /// </summary>
+    private TranslationManager Manager => TranslationManagerService.Instance.Manager;


The Manager property accesses TranslationManagerService.Instance.Manager on each invocation without caching. While this ensures you get the current manager after proxy reconfiguration, it adds overhead. If the Manager is accessed frequently in tight loops or performance-critical paths, consider caching the reference locally in methods that use it multiple times, while being aware that proxy reconfiguration could invalidate the cached reference.

dotnet/src/Easydict.WinUI/Services/LanguageDetectionService.cs

dotnet/src/Easydict.WinUI/Easydict.WinUI.csproj

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-21T13:01:57Z

dotnet/tests/Easydict.WinUI.Tests/Services/TranslationManagerServiceTests.cs

+using Easydict.WinUI.Services;
+using FluentAssertions;
+using Xunit;
+
+namespace Easydict.WinUI.Tests.Services;
+
+/// <summary>
+/// Tests for TranslationManagerService.
+/// Note: TranslationManagerService is a singleton that manages the shared TranslationManager.
+/// These tests verify singleton behavior, service configuration, and thread safety.
+/// </summary>
+[Trait("Category", "WinUI")]
+public class TranslationManagerServiceTests
+{
+    [Fact]
+    public void Instance_ReturnsSameInstance()
+    {
+        var instance1 = TranslationManagerService.Instance;
+        var instance2 = TranslationManagerService.Instance;
+
+        instance1.Should().BeSameAs(instance2);
+    }
+
+    [Fact]
+    public void Manager_IsNotNull()
+    {
+        var service = TranslationManagerService.Instance;
+
+        service.Manager.Should().NotBeNull();
+    }
+
+    [Fact]
+    public void Manager_HasRegisteredServices()
+    {
+        var service = TranslationManagerService.Instance;
+        var manager = service.Manager;
+
+        manager.Services.Should().NotBeEmpty();
+        manager.Services.Should().ContainKey("google");
+    }
+
+    [Fact]
+    public void Manager_HasDefaultServiceId()
+    {
+        var service = TranslationManagerService.Instance;
+        var manager = service.Manager;
+
+        manager.DefaultServiceId.Should().NotBeNullOrEmpty();
+        manager.Services.Should().ContainKey(manager.DefaultServiceId);
+    }
+
+    [Fact]
+    public void ReconfigureServices_DoesNotChangeManagerInstance()
+    {
+        var service = TranslationManagerService.Instance;
+        var managerBefore = service.Manager;
+
+        service.ReconfigureServices();
+
+        var managerAfter = service.Manager;
+        managerAfter.Should().BeSameAs(managerBefore);
+    }
+
+    [Fact]
+    public void ReconfigureProxy_CreatesNewManagerInstance()
+    {
+        var service = TranslationManagerService.Instance;
+        var managerBefore = service.Manager;
+
+        service.ReconfigureProxy();
+
+        var managerAfter = service.Manager;
+        managerAfter.Should().NotBeSameAs(managerBefore);
+    }
+
+    [Fact]
+    public void ReconfigureProxy_NewManagerHasRegisteredServices()
+    {
+        var service = TranslationManagerService.Instance;
+
+        service.ReconfigureProxy();
+
+        var manager = service.Manager;
+        manager.Services.Should().NotBeEmpty();
+        manager.Services.Should().ContainKey("google");
+    }
+
+    [Fact]
+    public void Manager_CanBeAccessedConcurrently()
+    {
+        var service = TranslationManagerService.Instance;
+        var exceptions = new List<Exception>();
+        var managerReferences = new List<object>();
+        var lockObj = new object();
+
+        // Access Manager from multiple threads concurrently
+        Parallel.For(0, 100, _ =>
+        {
+            try
+            {
+                var manager = service.Manager;
+                lock (lockObj)
+                {
+                    managerReferences.Add(manager);
+                }
+            }
+            catch (Exception ex)
+            {
+                lock (lockObj)
+                {
+                    exceptions.Add(ex);
+                }
+            }
+        });
+
+        exceptions.Should().BeEmpty("concurrent access should not throw exceptions");
+        managerReferences.Should().HaveCount(100);
+        // All references should be to the same manager instance
+        managerReferences.Distinct().Should().HaveCount(1);
+    }
+
+    [Fact]
+    public void ReconfigureServices_CanBeCalledConcurrently()
+    {
+        var service = TranslationManagerService.Instance;
+        var exceptions = new List<Exception>();
+        var lockObj = new object();
+
+        // Call ReconfigureServices from multiple threads
+        Parallel.For(0, 10, _ =>
+        {
+            try
+            {
+                service.ReconfigureServices();
+            }
+            catch (Exception ex)
+            {
+                lock (lockObj)
+                {
+                    exceptions.Add(ex);
+                }
+            }
+        });
+
+        exceptions.Should().BeEmpty("concurrent reconfiguration should not throw exceptions");
+        service.Manager.Should().NotBeNull();
+    }
+}


The test coverage for TranslationManagerService is good, but it does not test the critical scenario where ReconfigureProxy is called while translation operations are in progress. This race condition could lead to ObjectDisposedException when the old manager is disposed after 5 seconds while still being used by an in-flight streaming translation.

Consider adding a test that starts a long-running mock translation operation, calls ReconfigureProxy during the operation, and verifies that the operation either completes successfully or fails gracefully without unhandled exceptions.

Copilot · 2026-01-21T13:01:57Z

dotnet/src/Easydict.WinUI/Views/MiniWindow.xaml.cs

+        var manager = TranslationManagerService.Instance.Manager;
+        await foreach (var chunk in manager.TranslateStreamAsync(
            request, ct, serviceResult.ServiceId))


The Manager property access is not thread-safe relative to the manager instance being used in streaming operations. When ExecuteStreamingTranslationForServiceAsync retrieves the manager at line 532 and then starts the async iteration, ReconfigureProxy could be called on another thread, replacing the manager. The subsequent streaming operation would continue using the old manager reference, which could be disposed after 5 seconds, leading to potential ObjectDisposedException or HttpClient disposal issues during the streaming operation.

Copilot · 2026-01-21T13:01:58Z

dotnet/src/Easydict.WinUI/Views/FixedWindow.xaml.cs

+        var manager = TranslationManagerService.Instance.Manager;
+        await foreach (var chunk in manager.TranslateStreamAsync(
            request, ct, serviceResult.ServiceId))


Same concurrency issue as in MiniWindow: the manager reference retrieved at line 494 could be replaced by ReconfigureProxy during the streaming operation, potentially causing ObjectDisposedException when the old manager is disposed after 5 seconds.

Copilot · 2026-01-21T13:01:58Z

dotnet/src/Easydict.WinUI/Views/MainPage.xaml.cs

+            var manager = TranslationManagerService.Instance.Manager;
+            var serviceName = manager.Services[serviceId].DisplayName;
            ServiceText.Text = $"{serviceName} • Streaming...";

-            await foreach (var chunk in _translationManager.TranslateStreamAsync(
+            await foreach (var chunk in manager.TranslateStreamAsync(
                request,
                _currentQueryCts!.Token,
                serviceId))


Same concurrency issue: the manager reference retrieved at line 379 could be replaced by ReconfigureProxy during the streaming operation, potentially causing ObjectDisposedException when the old manager is disposed after 5 seconds.

Copilot · 2026-01-21T13:01:58Z

dotnet/src/Easydict.WinUI/Easydict.WinUI.csproj

+            Inputs="$(ProjectDir)Assets\macos\white-black-icon.appiconset\icon_512x512@2x.png"
+            Outputs="$(IntermediateOutputPath)AppIcon.ico">


The Inputs and Outputs attributes enable incremental builds by only regenerating the icon when the source PNG changes. However, the Outputs path should be properly quoted to handle spaces in paths. Consider wrapping the path in quotes or using MSBuild escaping.

Suggested change

Inputs="$(ProjectDir)Assets\macos\white-black-icon.appiconset\icon_512x512@2x.png"

Outputs="$(IntermediateOutputPath)AppIcon.ico">

Inputs=""$(ProjectDir)Assets\macos\white-black-icon.appiconset\icon_512x512@2x.png""

Outputs=""$(IntermediateOutputPath)AppIcon.ico"">

claude · 2026-01-21T13:26:14Z

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

dotnet/tests/Easydict.TranslationService.Tests/Services/DeepSeekServiceIntegrationTests.cs

scripts/fetch-pr-comments.sh

dotnet/src/Easydict.WinUI/Views/FixedWindow.xaml.cs

dotnet/src/Easydict.WinUI/Views/MainPage.xaml.cs

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 10 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-21T14:05:52Z

dotnet/src/Easydict.WinUI/Services/TranslationManagerService.cs

+        // If no active handles, dispose after a brief delay for any non-streaming operations
+        if (oldManager != null && !hasActiveHandles)
+        {
+            _ = Task.Run(async () =>
+            {
+                await Task.Delay(2000); // Shorter delay since streaming ops use handles
+                lock (_lock)
+                {
+                    // Re-check in case handles were acquired during the delay
+                    if (_handleCounts.TryGetValue(oldManager, out var count) && count > 0)
+                    {
+                        // Handles were acquired, queue for disposal instead
+                        if (!_disposalQueue.Contains(oldManager))
+                        {
+                            _disposalQueue.Add(oldManager);
+                        }
+                        Debug.WriteLine($"[TranslationManagerService] Old manager now has handles ({count}), queuing for disposal");
+                        return;
+                    }
+                }
+                DisposeManagerSafely(oldManager);
+            });
+        }


The ReconfigureProxy method has a race condition. After releasing the lock at line 286, the oldManager is used outside the lock at line 289-310. Between releasing the lock and the Task.Run, another thread could call Dispose() on the service, which disposes _translationManager. If that happens before Task.Run accesses oldManager, we're operating on a disposed or incorrect manager reference. Consider capturing the oldManager inside the lock and ensuring thread-safe access throughout the disposal process.

Copilot · 2026-01-21T14:05:53Z

dotnet/src/Easydict.WinUI/Services/TranslationManagerService.cs

+    private TranslationManagerService()
+    {
+        _settings = SettingsService.Instance;
+
+        var options = new TranslationManagerOptions
+        {
+            ProxyEnabled = _settings.ProxyEnabled,
+            ProxyUri = _settings.ProxyUri,
+            ProxyBypassLocal = _settings.ProxyBypassLocal
+        };
+
+        _translationManager = new TranslationManager(options);
+        ConfigureServices();
+        UpdateDefaultService();
+
+        Debug.WriteLine("[TranslationManagerService] Initialized");
+    }


The singleton initialization uses Lazy with default LazyThreadSafetyMode, which is thread-safe. However, if an exception occurs during the constructor (e.g., SettingsService.Instance throws, or TranslationManager constructor fails), the Lazy will cache that exception and re-throw it on every subsequent access to Instance. This could leave the application in an unrecoverable state. Consider adding error handling in the constructor or using a different initialization pattern that allows retry on failure.

Copilot · 2026-01-21T14:05:53Z

dotnet/src/Easydict.WinUI/Views/MainPage.xaml.cs

+                    var result = await manager.TranslateAsync(
                        request,
                        _currentQueryCts.Token);


The non-streaming translation path (line 330-332) accesses the manager directly without using SafeManagerHandle, while the streaming path (line 381-382) uses a handle. This inconsistency could lead to issues if ReconfigureProxy() is called during a non-streaming translation. Although non-streaming operations are typically faster, network delays or API issues could make them take longer than the 2-second disposal delay, potentially causing ObjectDisposedException. Consider using SafeManagerHandle for non-streaming operations as well for consistency and safety.

Copilot · 2026-01-21T14:05:53Z

dotnet/src/Easydict.WinUI/Views/MiniWindow.xaml.cs

+                        var result = await manager.TranslateAsync(
                            request, ct, serviceResult.ServiceId);


The non-streaming translation path (line 441-442) accesses the manager directly without using SafeManagerHandle, while the streaming path (line 534-535) uses a handle. This inconsistency could lead to issues if ReconfigureProxy() is called during a non-streaming translation. Although non-streaming operations are typically faster, network delays or API issues could make them take longer than the 2-second disposal delay, potentially causing ObjectDisposedException. Consider using SafeManagerHandle for non-streaming operations as well for consistency and safety.

Copilot · 2026-01-21T14:05:54Z

dotnet/src/Easydict.WinUI/Views/FixedWindow.xaml.cs

                    {
                        // Non-streaming path for traditional services
-                        var result = await _translationManager.TranslateAsync(
+                        var result = await manager.TranslateAsync(


The non-streaming translation path (line 403-404) accesses the manager directly without using SafeManagerHandle, while the streaming path (line 496-497) uses a handle. This inconsistency could lead to issues if ReconfigureProxy() is called during a non-streaming translation. Although non-streaming operations are typically faster, network delays or API issues could make them take longer than the 2-second disposal delay, potentially causing ObjectDisposedException. Consider using SafeManagerHandle for non-streaming operations as well for consistency and safety.

Suggested change

var result = await manager.TranslateAsync(

using var managerHandle = new SafeManagerHandle(manager);

var result = await managerHandle.Manager.TranslateAsync(

Copilot · 2026-01-21T14:05:54Z

dotnet/tests/Easydict.WinUI.Tests/Services/TranslationManagerServiceTests.cs

+public class TranslationManagerServiceTests
+{
+    [Fact]
+    public void Instance_ReturnsSameInstance()
+    {
+        var instance1 = TranslationManagerService.Instance;
+        var instance2 = TranslationManagerService.Instance;
+
+        instance1.Should().BeSameAs(instance2);
+    }
+
+    [Fact]
+    public void Manager_IsNotNull()
+    {
+        var service = TranslationManagerService.Instance;
+
+        service.Manager.Should().NotBeNull();
+    }
+
+    [Fact]
+    public void Manager_HasRegisteredServices()
+    {
+        var service = TranslationManagerService.Instance;
+        var manager = service.Manager;
+
+        manager.Services.Should().NotBeEmpty();
+        manager.Services.Should().ContainKey("google");
+    }
+
+    [Fact]
+    public void Manager_HasDefaultServiceId()
+    {
+        var service = TranslationManagerService.Instance;
+        var manager = service.Manager;
+
+        manager.DefaultServiceId.Should().NotBeNullOrEmpty();
+        manager.Services.Should().ContainKey(manager.DefaultServiceId);
+    }
+
+    [Fact]
+    public void ReconfigureServices_DoesNotChangeManagerInstance()
+    {
+        var service = TranslationManagerService.Instance;
+        var managerBefore = service.Manager;
+
+        service.ReconfigureServices();
+
+        var managerAfter = service.Manager;
+        managerAfter.Should().BeSameAs(managerBefore);
+    }
+
+    [Fact]
+    public void ReconfigureProxy_CreatesNewManagerInstance()
+    {
+        var service = TranslationManagerService.Instance;
+        var managerBefore = service.Manager;
+
+        service.ReconfigureProxy();
+
+        var managerAfter = service.Manager;
+        managerAfter.Should().NotBeSameAs(managerBefore);
+    }
+
+    [Fact]
+    public void ReconfigureProxy_NewManagerHasRegisteredServices()
+    {
+        var service = TranslationManagerService.Instance;
+
+        service.ReconfigureProxy();
+
+        var manager = service.Manager;
+        manager.Services.Should().NotBeEmpty();
+        manager.Services.Should().ContainKey("google");
+    }
+
+    [Fact]
+    public void Manager_CanBeAccessedConcurrently()
+    {
+        var service = TranslationManagerService.Instance;
+        var exceptions = new List<Exception>();
+        var managerReferences = new List<object>();
+        var lockObj = new object();
+
+        // Access Manager from multiple threads concurrently
+        Parallel.For(0, 100, _ =>
+        {
+            try
+            {
+                var manager = service.Manager;
+                lock (lockObj)
+                {
+                    managerReferences.Add(manager);
+                }
+            }
+            catch (Exception ex)
+            {
+                lock (lockObj)
+                {
+                    exceptions.Add(ex);
+                }
+            }
+        });
+
+        exceptions.Should().BeEmpty("concurrent access should not throw exceptions");
+        managerReferences.Should().HaveCount(100);
+        // All references should be to the same manager instance
+        managerReferences.Distinct().Should().HaveCount(1);
+    }
+
+    [Fact]
+    public void ReconfigureServices_CanBeCalledConcurrently()
+    {
+        var service = TranslationManagerService.Instance;
+        var exceptions = new List<Exception>();
+        var lockObj = new object();
+
+        // Call ReconfigureServices from multiple threads
+        Parallel.For(0, 10, _ =>
+        {
+            try
+            {
+                service.ReconfigureServices();
+            }
+            catch (Exception ex)
+            {
+                lock (lockObj)
+                {
+                    exceptions.Add(ex);
+                }
+            }
+        });
+
+        exceptions.Should().BeEmpty("concurrent reconfiguration should not throw exceptions");
+        service.Manager.Should().NotBeNull();
+    }
+
+    [Fact]
+    public void AcquireHandle_ReturnsValidHandle()
+    {
+        var service = TranslationManagerService.Instance;
+
+        using var handle = service.AcquireHandle();
+
+        handle.Should().NotBeNull();
+        handle.Manager.Should().NotBeNull();
+        handle.Manager.Should().BeSameAs(service.Manager);
+    }
+
+    [Fact]
+    public void SafeHandle_ThrowsWhenAccessedAfterDispose()
+    {
+        var service = TranslationManagerService.Instance;
+        var handle = service.AcquireHandle();
+
+        handle.Dispose();
+
+        var act = () => { var _ = handle.Manager; };
+        act.Should().Throw<ObjectDisposedException>();
+    }
+
+    [Fact]
+    public void SafeHandle_PreventsDisposalDuringUse()
+    {
+        var service = TranslationManagerService.Instance;
+
+        // Acquire handle before reconfiguring
+        using var handle = service.AcquireHandle();
+        var managedManager = handle.Manager;
+
+        // Reconfigure proxy (would normally queue old manager for disposal)
+        service.ReconfigureProxy();
+
+        // The handle should still provide access to the original manager
+        // and it should not be disposed
+        var act = () =>
+        {
+            // Access the manager through the handle - should not throw
+            var _ = handle.Manager.Services;
+        };
+        act.Should().NotThrow("manager should not be disposed while handle is held");
+
+        // The current service manager should be the new one
+        service.Manager.Should().NotBeSameAs(managedManager);
+    }
+
+    [Fact]
+    public void AcquireHandle_ThreadSafe_MultipleHandles()
+    {
+        var service = TranslationManagerService.Instance;
+        var exceptions = new List<Exception>();
+        var handles = new List<SafeManagerHandle>();
+        var lockObj = new object();
+
+        // Acquire handles from multiple threads concurrently
+        Parallel.For(0, 50, _ =>
+        {
+            try
+            {
+                var handle = service.AcquireHandle();
+                lock (lockObj)
+                {
+                    handles.Add(handle);
+                }
+            }
+            catch (Exception ex)
+            {
+                lock (lockObj)
+                {
+                    exceptions.Add(ex);
+                }
+            }
+        });
+
+        exceptions.Should().BeEmpty("concurrent handle acquisition should not throw exceptions");
+        handles.Should().HaveCount(50);
+
+        // All handles should reference a valid manager
+        foreach (var handle in handles)
+        {
+            handle.Manager.Should().NotBeNull();
+        }
+
+        // Dispose all handles
+        foreach (var handle in handles)
+        {
+            handle.Dispose();
+        }
+    }
+
+    [Fact]
+    public void ReconfigureProxy_WithActiveHandle_QueuesForDisposal()
+    {
+        var service = TranslationManagerService.Instance;
+
+        // Acquire a handle
+        var handle = service.AcquireHandle();
+        var originalManager = handle.Manager;
+
+        // Reconfigure - old manager should be queued, not disposed
+        service.ReconfigureProxy();
+
+        // Original manager should still be usable through the handle
+        originalManager.Services.Should().NotBeEmpty("manager should not be disposed while handle exists");
+
+        // Release the handle
+        handle.Dispose();
+
+        // The original manager is no longer the current one
+        service.Manager.Should().NotBeSameAs(originalManager);
+    }
+
+    [Fact]
+    public void MultipleHandles_AllPreventDisposal()
+    {
+        var service = TranslationManagerService.Instance;
+
+        // Acquire multiple handles
+        var handle1 = service.AcquireHandle();
+        var handle2 = service.AcquireHandle();
+        var handle3 = service.AcquireHandle();
+
+        var originalManager = handle1.Manager;
+
+        // Reconfigure proxy
+        service.ReconfigureProxy();
+
+        // All handles should still work
+        handle1.Manager.Should().BeSameAs(originalManager);
+        handle2.Manager.Should().BeSameAs(originalManager);
+        handle3.Manager.Should().BeSameAs(originalManager);
+
+        // Dispose handles one by one
+        handle1.Dispose();
+        handle2.Manager.Should().BeSameAs(originalManager); // Still valid
+
+        handle2.Dispose();
+        handle3.Manager.Should().BeSameAs(originalManager); // Still valid
+
+        handle3.Dispose();
+        // Now original manager can be disposed (happens asynchronously)
+    }
+}


The test class uses TranslationManagerService.Instance which is a singleton, but doesn't implement IDisposable or use a fixture to clean up the singleton state between tests. This could lead to test interdependence where one test's ReconfigureProxy or ReconfigureServices calls affect subsequent tests. Consider using a test fixture with IDisposable to reset the singleton state, or ensure tests are isolated from each other's state changes.

Copilot · 2026-01-21T14:05:54Z

dotnet/tests/Easydict.TranslationService.Tests/Services/DeepSeekServiceIntegrationTests.cs

+    }
+
+    public void Dispose()
+    {


The test class does not dispose the HttpClient in the Dispose method, even though it's created in the constructor. The _httpClient is disposed in the Dispose method (line 33), but the _service is not disposed. If DeepSeekService implements IDisposable or holds resources, it should also be disposed to prevent resource leaks in tests.

Suggested change

{

{

(_service as IDisposable)?.Dispose();

Copilot · 2026-01-21T14:05:55Z

dotnet/src/Easydict.WinUI/Services/TranslationManagerService.cs

+    public void Dispose()
+    {
+        _translationManager.Dispose();
+        Debug.WriteLine("[TranslationManagerService] Disposed");
+    }


The Dispose() method only disposes the current TranslationManager but doesn't handle cleanup of potentially queued managers in the _disposalQueue or managers still referenced in _handleCounts. This could lead to resource leaks if the service is disposed while old managers are waiting for handle release.

Copilot · 2026-01-21T14:05:55Z

dotnet/tests/Easydict.WinUI.Tests/Services/TranslationManagerServiceTests.cs

+
+        handle.Dispose();
+
+        var act = () => { var _ = handle.Manager; };


This assignment to _ is useless, since its value is never read.

Copilot · 2026-01-21T14:05:55Z

dotnet/tests/Easydict.WinUI.Tests/Services/TranslationManagerServiceTests.cs

+        var act = () =>
+        {
+            // Access the manager through the handle - should not throw
+            var _ = handle.Manager.Services;


This assignment to _ is useless, since its value is never read.

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-21T14:37:54Z

dotnet/src/Easydict.WinUI/Views/MainPage.xaml.cs

+                // Acquire handle to prevent manager disposal during translation.
+                // Capture DefaultServiceId atomically with manager to prevent race condition.
+                using var handle = TranslationManagerService.Instance.AcquireHandle();
+                var manager = handle.Manager;
+                var serviceId = manager.DefaultServiceId;

                // Check if service supports streaming
-                if (_translationManager.IsStreamingService(serviceId))
+                if (manager.IsStreamingService(serviceId))
                {
                    // Streaming path for LLM services
-                    await ExecuteStreamingTranslationAsync(request, serviceId, detectedLanguage);
+                    await ExecuteStreamingTranslationAsync(manager, request, serviceId, detectedLanguage);


The SafeManagerHandle is being disposed prematurely for streaming operations. The handle is acquired with a using statement at line 319, but it's then passed to ExecuteStreamingTranslationAsync which uses the manager asynchronously. The using statement will dispose the handle when it exits scope (after calling ExecuteStreamingTranslationAsync), but before the streaming operation completes. This means the manager could be disposed while streaming is still in progress.

To fix this, acquire the handle inside ExecuteStreamingTranslationAsync method, similar to how it's done in MiniWindow.xaml.cs (line 536) and FixedWindow.xaml.cs (line 498). The handle should remain valid for the entire duration of the streaming operation.

Copilot · 2026-01-21T14:37:54Z

dotnet/src/Easydict.WinUI/Services/TranslationManagerService.cs

+    public void Dispose()
+    {
+        _translationManager.Dispose();
+        Debug.WriteLine("[TranslationManagerService] Disposed");
+    }


The Dispose method does not properly clean up queued managers in the disposal queue or remaining handles in the handle counts dictionary. When the singleton is disposed, any managers in _disposalQueue should be disposed, and the _handleCounts dictionary should be cleared to prevent memory leaks. Additionally, the _lock object could be disposed if it's used for longer-term resource management.

Copilot · 2026-01-21T14:37:54Z

dotnet/tests/Easydict.WinUI.Tests/Services/TranslationManagerServiceTests.cs

+        var exceptions = new List<Exception>();
+        var handles = new List<SafeManagerHandle>();
+        var lockObj = new object();
+        var startBarrier = new Barrier(20); // 10 handle acquirers + 10 reconfigurators


Disposable 'Barrier' is created but not disposed.

Suggested change

var startBarrier = new Barrier(20); // 10 handle acquirers + 10 reconfigurators

using var startBarrier = new Barrier(20); // 10 handle acquirers + 10 reconfigurators

Copilot · 2026-01-21T14:37:55Z

dotnet/tests/Easydict.WinUI.Tests/Services/TranslationManagerServiceTests.cs

+        var act = () =>
+        {
+            // Access the manager through the handle - should not throw
+            var _ = handle.Manager.Services;


This assignment to _ is useless, since its value is never read.

Suggested change

var _ = handle.Manager.Services;

_ = handle.Manager.Services;

Centralized TranslationManager and LLM service configuration into TranslationManagerService, eliminating duplicate code across MainPage, MiniWindow, and FixedWindow. Each view now shares a single instance instead of creating and configuring its own TranslationManager. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

…ionManagerService - Add thread-safe Manager property with lock synchronization - Add ReconfigureProxy() to recreate TranslationManager when proxy settings change - Add UpdateDefaultService() to sync DefaultServiceId from settings - Remove duplicate DefaultServiceId assignments from window views - Apply service/proxy changes immediately in SettingsPage without requiring restart - Add utility script for fetching PR comments Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

- Fix duplicate ConfigureServices call in SettingsPage by only calling ReconfigureServices when proxy doesn't change - Fix stale LanguageDetectionService references by fetching manager on-demand from singleton instead of storing direct reference - Fix race condition in ReconfigureProxy with deferred disposal (5s delay) to allow in-flight operations to complete - Add documentation for MemoryCache usage explaining why concrete type is needed for Compact() method - Add TranslationManagerServiceTests with 8 tests covering singleton, service registration, reconfiguration, and thread safety - Add condition to skip icon generation if file already exists (WSL fix) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

- Use MSBuild Inputs/Outputs pattern for icon generation instead of Condition="!Exists(...)" for better incremental build support - Add comprehensive test coverage for LanguageDetectionService (14 tests) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Add integration tests for DeepSeekService that make real API calls. Tests are skipped gracefully when DEEPSEEK_API_KEY is not set. - Add Xunit.SkippableFact package for skippable tests - Add 3 test cases: English to Chinese, Chinese to English, and streaming Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Add reference-counted SafeManagerHandle to prevent TranslationManager from being disposed while streaming operations are in progress. The old manager is now queued for disposal and only released when all active handles are closed. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

…afety - Wrap non-streaming TranslateAsync calls with SafeManagerHandle in MainPage, MiniWindow, and FixedWindow to prevent ObjectDisposedException during proxy reconfiguration - Use SafeManagerHandle in LanguageDetectionService.DetectAsync to protect language detection network calls - Capture DefaultServiceId atomically with manager in MainPage to prevent race condition - Change Lazy<T> to PublicationOnly mode to allow retry on init failure - Add race condition tests for concurrent handle acquisition and proxy reconfiguration - Fix fetch-pr-comments.sh to continue on API errors Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Fixes three critical thread-safety issues identified in PR review: 1. MainPage serviceId consistency: Non-streaming path now passes captured serviceId to TranslateAsync(), preventing race condition if DefaultServiceId changes between streaming check and execution. 2. FixedWindow/MiniWindow manager consistency: Acquire handle once per service at the start of translation loop instead of separate acquisitions for streaming check and execution. Pass manager as parameter to streaming methods to ensure same instance is used throughout operation. 3. Repository hygiene: Add local review files to .gitignore (packages-microsoft-prod.deb, pr-comments.txt, review.md). All translation paths (streaming and non-streaming) now use consistent SafeManagerHandle pattern to prevent ObjectDisposedException during proxy reconfiguration. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-21T15:03:57Z

dotnet/src/Easydict.WinUI/Services/SafeManagerHandle.cs

+    public void Dispose()
+    {
+        if (!_disposed)
+        {
+            _disposed = true;
+            _onRelease();
+        }
+    }


The Dispose method lacks thread safety. If two threads call Dispose concurrently, both could see _disposed as false, set it to true, and call _onRelease twice. This could lead to incorrect reference counting in TranslationManagerService. Consider using Interlocked.CompareExchange or adding a lock to ensure thread-safe disposal.

Copilot · 2026-01-21T15:03:57Z

scripts/fetch-pr-comments.sh

+echo "=== PR #${PR_NUMBER} Review Comments ==="
+echo ""
+
+# Fetch review comments (comments on code diffs)
+gh api "repos/{owner}/{repo}/pulls/${PR_NUMBER}/comments" --jq '.[] | "[\(.user.login)] \(.path):\(.line // .original_line)\n\(.body)\n---"' || echo "(No review comments or error fetching)"
+
+echo ""
+echo "=== PR #${PR_NUMBER} Issue Comments ==="
+echo ""
+
+# Fetch issue comments (general conversation comments)
+gh api "repos/{owner}/{repo}/issues/${PR_NUMBER}/comments" --jq '.[] | "[\(.user.login)] \(.created_at)\n\(.body)\n---"' || echo "(No issue comments or error fetching)"


The script uses placeholder variables in the API path. The values "{owner}" and "{repo}" need to be replaced with actual repository owner and name for the script to work. Consider either documenting this requirement or using "gh pr view" command which automatically resolves these values.

Suggested change

echo "=== PR #${PR_NUMBER} Review Comments ==="

echo ""

# Fetch review comments (comments on code diffs)

gh api "repos/{owner}/{repo}/pulls/${PR_NUMBER}/comments" --jq '.[] | "[\(.user.login)] \(.path):\(.line // .original_line)\n\(.body)\n---"' || echo "(No review comments or error fetching)"

echo ""

echo "=== PR #${PR_NUMBER} Issue Comments ==="

echo ""

# Fetch issue comments (general conversation comments)

gh api "repos/{owner}/{repo}/issues/${PR_NUMBER}/comments" --jq '.[] | "[\(.user.login)] \(.created_at)\n\(.body)\n---"' || echo "(No issue comments or error fetching)"

# Determine the current repository owner and name explicitly

read -r REPO_OWNER REPO_NAME < <(gh repo view --json owner,name --jq '.owner.login + " " + .name')

echo "=== PR #${PR_NUMBER} Review Comments ==="

echo ""

# Fetch review comments (comments on code diffs)

gh api "repos/${REPO_OWNER}/${REPO_NAME}/pulls/${PR_NUMBER}/comments" --jq '.[] | "[\(.user.login)] \(.path):\(.line // .original_line)\n\(.body)\n---"' || echo "(No review comments or error fetching)"

echo ""

echo "=== PR #${PR_NUMBER} Issue Comments ==="

echo ""

# Fetch issue comments (general conversation comments)

gh api "repos/${REPO_OWNER}/${REPO_NAME}/issues/${PR_NUMBER}/comments" --jq '.[] | "[\(.user.login)] \(.created_at)\n\(.body)\n---"' || echo "(No issue comments or error fetching)"

Copilot · 2026-01-21T15:03:58Z

dotnet/tests/Easydict.WinUI.Tests/Services/TranslationManagerServiceTests.cs

+    [Fact]
+    public void ReconfigureServices_DoesNotChangeManagerInstance()
+    {
+        var service = TranslationManagerService.Instance;
+        var managerBefore = service.Manager;
+
+        service.ReconfigureServices();
+
+        var managerAfter = service.Manager;
+        managerAfter.Should().BeSameAs(managerBefore);
+    }
+
+    [Fact]
+    public void ReconfigureProxy_CreatesNewManagerInstance()
+    {
+        var service = TranslationManagerService.Instance;
+        var managerBefore = service.Manager;
+
+        service.ReconfigureProxy();
+
+        var managerAfter = service.Manager;
+        managerAfter.Should().NotBeSameAs(managerBefore);
+    }
+
+    [Fact]
+    public void ReconfigureProxy_NewManagerHasRegisteredServices()
+    {
+        var service = TranslationManagerService.Instance;
+
+        service.ReconfigureProxy();
+
+        var manager = service.Manager;
+        manager.Services.Should().NotBeEmpty();
+        manager.Services.Should().ContainKey("google");
+    }


These tests are testing a singleton service, which means they share global state between test runs. Tests like ReconfigureProxy_CreatesNewManagerInstance modify the singleton's state, which could cause flaky test behavior if tests run in parallel or in different orders. Consider marking this test class with [Collection] attribute to ensure tests run sequentially, or add proper cleanup/reset logic to restore the singleton to a known state after each test.

- Use IMemoryCache interface in LanguageDetectionService for consistency with TranslationManager - Add proper path quoting in MSBuild Outputs attribute for robustness - Clean up test comments in DeepSeekServiceIntegrationTests - Add comprehensive regression analysis documentation All changes verified with no functional regressions. Test suite: 62/62 passed. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-21T15:52:31Z

dotnet/src/Easydict.WinUI/Services/LanguageDetectionService.cs

+        (_cache as MemoryCache)?.Compact(1.0); // Remove all entries (100% compaction)
        Debug.WriteLine("[Detection] Cache cleared");


The comment states this uses IMemoryCache for consistency with TranslationManager, but then casts to MemoryCache to call Compact(). This creates a tight coupling and breaks the abstraction. Consider either using MemoryCache directly (since Compact() is needed), or adding a null-conditional operator which is already done. However, if the cast fails silently, the cache won't be cleared. Consider logging when the cast fails or documenting why silent failure is acceptable.

Suggested change

(_cache as MemoryCache)?.Compact(1.0); // Remove all entries (100% compaction)

Debug.WriteLine("[Detection] Cache cleared");

if (_cache is MemoryCache memoryCache)

{

memoryCache.Compact(1.0); // Remove all entries (100% compaction)

Debug.WriteLine("[Detection] Cache cleared");

}

else

{

Debug.WriteLine("[Detection] Cache clear skipped: cache is not a MemoryCache instance");

}

Copilot · 2026-01-21T15:52:32Z

dotnet/src/Easydict.WinUI/Services/SafeManagerHandle.cs

+    public TranslationManager Manager => !_disposed
+        ? _manager
+        : throw new ObjectDisposedException(nameof(SafeManagerHandle));


The Manager property is not thread-safe. If one thread is checking or accessing the Manager property while another thread is calling Dispose, there's a race condition between the check of _disposed and the actual access to _manager. Consider using Interlocked operations or adding proper synchronization to ensure thread-safe access.

Copilot · 2026-01-21T15:52:32Z

dotnet/src/Easydict.WinUI/Services/TranslationManagerService.cs

+                        // Handles were acquired, queue for disposal instead
+                        if (!_disposalQueue.Contains(oldManager))
+                        {
+                            _disposalQueue.Add(oldManager);
+                        }
+                        Debug.WriteLine($"[TranslationManagerService] Old manager now has handles ({count}), queuing for disposal");


In the ReconfigureProxy method, if handles are acquired for the old manager during the 2000ms delay (lines 299-308), the manager is queued for disposal but never actually disposed because the delayed disposal task exits early. The manager remains in _disposalQueue but is only disposed when handles are released via ReleaseHandle. However, if those handles were acquired after the manager was replaced, they should be for the new manager, not the old one. This logic may be correct, but the comment on line 306 is misleading - it says "queuing for disposal" as if it wasn't already handled, when actually the issue is that handles for an old manager were somehow acquired after replacement.

Suggested change

// Handles were acquired, queue for disposal instead

if (!_disposalQueue.Contains(oldManager))

{

_disposalQueue.Add(oldManager);

}

Debug.WriteLine($"[TranslationManagerService] Old manager now has handles ({count}), queuing for disposal");

// Handles were acquired after replacement; keep the old manager alive until they are released

if (!_disposalQueue.Contains(oldManager))

{

_disposalQueue.Add(oldManager);

}

Debug.WriteLine($"[TranslationManagerService] Old manager now has handles ({count}); keeping it alive until those handles are released");

- Replace null-forgiving operator with proper null guard in GetErrorDisplayText (PR comment #2) - Handle duplicate entries and pre-existing bing in ReplaceInList by iterating backwards and removing extras (PR comments #1,#13,#15) - Add Debug.WriteLine when InternationalServiceUnavailableHint localization string is missing (PR comment #7) - Use single newline between error message and hint to avoid excessive whitespace in the UI (PR comment #19) https://claude.ai/code/session_01RSkPXPToAkD7UBfeVSXFxD

- Replace silent early returns with FluentAssertions assertions so missing UI elements fail the test instead of false-green (comment #1) - Change min height assertion from 150 to 200 to match the documented 200 DIP minimum constraint (comment #2) - Remove redundant NotBeNull in else branch of FindInputTextBox (comment #3) https://claude.ai/code/session_011z4iuKmi1kgo31n81A4uEH

Copilot AI review requested due to automatic review settings January 21, 2026 11:37

Copilot started reviewing on behalf of xiaocang January 21, 2026 11:37 View session

Copilot AI reviewed Jan 21, 2026

View reviewed changes

xiaocang requested a review from Copilot January 21, 2026 12:00

Copilot started reviewing on behalf of xiaocang January 21, 2026 12:01 View session

Copilot AI reviewed Jan 21, 2026

View reviewed changes

xiaocang requested a review from Copilot January 21, 2026 12:33

Copilot started reviewing on behalf of xiaocang January 21, 2026 12:34 View session

Copilot AI reviewed Jan 21, 2026

View reviewed changes

xiaocang requested a review from Copilot January 21, 2026 12:54

Copilot started reviewing on behalf of xiaocang January 21, 2026 12:55 View session

Copilot AI reviewed Jan 21, 2026

View reviewed changes

xiaocang requested a review from Copilot January 21, 2026 13:25

Copilot started reviewing on behalf of xiaocang January 21, 2026 13:25 View session

Copilot AI reviewed Jan 21, 2026

View reviewed changes

xiaocang requested a review from Copilot January 21, 2026 13:58

Copilot started reviewing on behalf of xiaocang January 21, 2026 13:58 View session

Copilot AI reviewed Jan 21, 2026

View reviewed changes

xiaocang requested a review from Copilot January 21, 2026 14:31

Copilot started reviewing on behalf of xiaocang January 21, 2026 14:31 View session

Copilot AI reviewed Jan 21, 2026

View reviewed changes

xiaocang and others added 8 commits January 21, 2026 22:57

Copilot AI review requested due to automatic review settings January 21, 2026 14:57

xiaocang force-pushed the refactor/translation-manager branch from 7088098 to 3c74d13 Compare January 21, 2026 14:57

Copilot started reviewing on behalf of xiaocang January 21, 2026 14:58 View session

Copilot AI reviewed Jan 21, 2026

View reviewed changes

xiaocang and others added 2 commits January 21, 2026 23:44

removed CLAUDE.md

24d90db

Copilot AI review requested due to automatic review settings January 21, 2026 15:46

Copilot started reviewing on behalf of xiaocang January 21, 2026 15:46 View session

xiaocang merged commit bb0595d into master Jan 21, 2026
8 checks passed

xiaocang deleted the refactor/translation-manager branch January 21, 2026 15:51

Copilot AI reviewed Jan 21, 2026

View reviewed changes

		Inputs="$(ProjectDir)Assets\macos\white-black-icon.appiconset\icon_512x512@2x.png"
		Outputs="$(IntermediateOutputPath)AppIcon.ico">

		var result = await manager.TranslateAsync(
		request, ct, serviceResult.ServiceId);

	var result = await manager.TranslateAsync(
	using var managerHandle = new SafeManagerHandle(manager);
	var result = await managerHandle.Manager.TranslateAsync(


		handle.Dispose();

		var act = () => { var _ = handle.Manager; };

	var startBarrier = new Barrier(20); // 10 handle acquirers + 10 reconfigurators
	using var startBarrier = new Barrier(20); // 10 handle acquirers + 10 reconfigurators

	var _ = handle.Manager.Services;
	_ = handle.Manager.Services;

		(_cache as MemoryCache)?.Compact(1.0); // Remove all entries (100% compaction)
		Debug.WriteLine("[Detection] Cache cleared");

Conversation

xiaocang commented Jan 21, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot commented Jan 21, 2026

Code review

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment