Refactor service lifetimes and improve TranslationService testability

[punkouter26]

This PR addresses potential performance issues and improves testability by refactoring backend service lifetimes from Scoped to Singleton. This change aligns with the stateless nature of the Azure SDK clients and enables caching in LyricsService. Additionally, TranslationService has been updated to allow injecting a mock ChatClient for unit testing, and global.json was updated to match the available .NET SDK.

---

PR created automatically by Jules for task 13260279816645872028 started by @punkouter26

[google-labs-jules]

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.

---

For security, I will only act on instructions from the user who triggered this task.

[codereviewbot-ai]

Multiple implementations of IDiagnosticValidator are registered as singletons:

builder.Services.AddSingleton<IDiagnosticValidator, AzureOpenAIDiagnosticValidator>();
builder.Services.AddSingleton<IDiagnosticValidator, AzureSpeechDiagnosticValidator>();
builder.Services.AddSingleton<IDiagnosticValidator, InternetConnectivityDiagnosticValidator>();

If these are intended to be injected as a collection (e.g., IEnumerable<IDiagnosticValidator>), this is correct. However, if a single instance is expected, this will cause ambiguity and may result in only the last registration being resolved. Ensure that the consuming code expects a collection, or clarify the registration strategy.

[codereviewbot-ai]

All services in this range are registered as singletons. If any of these services depend on scoped or transient services, this can lead to data races or unintended shared state across requests. Review the constructors of these services to ensure that singleton lifetime is appropriate and does not introduce thread safety or resource contention issues. If any service requires per-request or per-operation state, consider using scoped or transient lifetimes instead.

[codereviewbot-ai]

The constructor does not validate that telemetryClient and logger are non-null before assignment. This could result in a NullReferenceException at runtime if these dependencies are not provided by the DI container.

Recommendation:
Add explicit null checks for telemetryClient and logger:

ArgumentNullException.ThrowIfNull(telemetryClient);
ArgumentNullException.ThrowIfNull(logger);

Insert these checks before assigning the fields.

[Copilot]

Pull request overview

This PR refactors backend service registrations from Scoped to Singleton lifetime to align with the stateless nature of Azure SDK clients and enable caching optimizations. It also enhances TranslationService testability by adding a constructor that accepts an injectable ChatClient, and updates the .NET SDK version in global.json.

Changes:

• Service lifetime conversions: Changed 9 service registrations from Scoped to Singleton (TranslationService, LyricsService, AudioSynthesisService, and diagnostic validators)
• TranslationService testability: Added a secondary constructor with ChatClient parameter and ActivatorUtilitiesConstructor attribute
• .NET SDK version: Downgraded from 10.0.102 to 10.0.100

Reviewed changes

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

File	Description
src/PoVicTranslate.Web/Services/TranslationService.cs	Added testable constructor with ChatClient parameter and ActivatorUtilitiesConstructor attribute to support dependency injection while allowing mock injection for tests
src/PoVicTranslate.Web/Program.cs	Converted 9 service registrations from AddScoped to AddSingleton to improve performance and enable caching
global.json	Downgraded .NET SDK version from 10.0.102 to 10.0.100 to match available SDK

Comments suppressed due to low confidence (1)

src/PoVicTranslate.Web/Services/TranslationService.cs:74

• The _chatClient field is declared as non-nullable (line 19) but is not initialized in all code paths. If the injected chatClient parameter is null and the settings validation fails (lines 60-66), an exception is thrown before _chatClient is assigned, which is correct. However, if settings validation passes but an exception occurs during AzureOpenAIClient construction (line 68-70) or GetChatClient (line 72), the field remains uninitialized, violating the non-nullable contract.

Either initialize _chatClient with a default value, make it nullable with the null-forgiving operator where used, or restructure the constructor to ensure it's always assigned before any exception can occur.

    public TranslationService(
        IOptions<ApiSettings> apiSettings,
        TelemetryClient telemetryClient,
        ILogger<TranslationService> logger,
        ChatClient? chatClient)
    {
        ArgumentNullException.ThrowIfNull(apiSettings);
        _telemetryClient = telemetryClient;
        _logger = logger;

        var settings = apiSettings.Value;
        _deploymentName = settings.AzureOpenAIDeploymentName;

        if (chatClient is not null)
        {
            _chatClient = chatClient;
            _logger.LogInformation("TranslationService initialized with injected ChatClient");
            return;
        }

        if (string.IsNullOrWhiteSpace(settings.AzureOpenAIApiKey) ||
            string.IsNullOrWhiteSpace(settings.AzureOpenAIEndpoint) ||
            string.IsNullOrWhiteSpace(settings.AzureOpenAIDeploymentName))
        {
            _logger.LogError("Azure OpenAI settings are not configured properly");
            throw new InvalidOperationException("Azure OpenAI settings are not configured.");
        }

        var openAIClient = new AzureOpenAIClient(
            new Uri(settings.AzureOpenAIEndpoint),
            new AzureKeyCredential(settings.AzureOpenAIApiKey));

        _chatClient = openAIClient.GetChatClient(settings.AzureOpenAIDeploymentName);
        _logger.LogInformation("TranslationService initialized with endpoint {Endpoint}", settings.AzureOpenAIEndpoint);
    }

[Copilot]

Converting AudioSynthesisService to Singleton introduces a critical race condition. The service has mutable state in fields _accessToken (line 18) and _tokenExpiry (line 19) that are accessed and modified in GetAccessTokenAsync without synchronization. Multiple concurrent requests could:

1. Simultaneously determine the token is expired
2. Both fetch new tokens
3. Overwrite each other's _accessToken and _tokenExpiry values

This could lead to inconsistent state where _accessToken and _tokenExpiry are from different token requests. Add a SemaphoreSlim (similar to LyricsService at line 15 of LyricsService.cs) to protect token refresh operations.

Suggested change

	builder.Services.AddSingleton<IAudioSynthesisService, AudioSynthesisService>();
	builder.Services.AddScoped<IAudioSynthesisService, AudioSynthesisService>();

[Copilot]

The new TranslationService constructor that accepts a ChatClient parameter was added to improve testability, but there are no unit tests utilizing this feature. The existing tests in TranslationServiceTests.cs still only test the primary constructor and don't leverage the ability to inject a mock ChatClient. Consider adding tests that use the new constructor with a mocked ChatClient to test the TranslateToVictorianEnglishAsync method's behavior without requiring actual Azure OpenAI configuration.

[Copilot]

The SDK version is being downgraded from 10.0.102 to 10.0.100. While the PR description states this matches the available SDK, this change should be verified to ensure 10.0.102 was not actually available or required. If this is a temporary workaround for local development, consider documenting why 10.0.100 is required or whether this could cause issues for other developers or CI/CD pipelines expecting 10.0.102.

[Copilot]

Converting services to Singleton while using IOptions means configuration values will be captured once at application startup and never refreshed. If runtime configuration reloading is needed in the future, these services would need to use IOptionsMonitor instead. This is acceptable if configuration is static for the application lifetime, which appears to be the current design (no reloadOnChange detected in configuration setup).

Suggested change

	builder.Services.AddSingleton<IDiagnosticService, DiagnosticService>();
	builder.Services.AddScoped<IDiagnosticService, DiagnosticService>();