Fix lyrics JSON deserialization and service lifetimes #4

punkouter26 · 2026-01-31T15:26:38Z

Fixed several issues in the codebase:

Corrected LyricsService to correctly deserialize JSON by adding [JsonPropertyName("content")] to SongEntry and enabling case-insensitive property matching.
Changed LyricsService and other backend services to Singleton lifetime to enable caching and optimize resource usage.
Removed incorrect file path manipulation in LyricsService.GetLyricsAsync which caused issues with song titles containing dots.
Updated integration test GetAvailableSongs_ShouldReturnList to use the correct API endpoint /api/lyrics/titles.
Updated global.json to match the installed .NET SDK version.

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

Co-authored-by: punkouter26 <[email protected]>

google-labs-jules · 2026-01-31T15:26:40Z

👋 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 · 2026-01-31T15:27:15Z

src/PoVicTranslate.Web/Program.cs

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


Multiple implementations of IDiagnosticValidator are registered as singletons:

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

Ensure that all consumers of IDiagnosticValidator are designed to resolve IEnumerable<IDiagnosticValidator>. If any consumer expects a single instance, this will cause runtime errors or unexpected behavior. Review all usages of IDiagnosticValidator to confirm correct resolution.

Recommended solution:

If multiple implementations are intended, ensure all consumers use IEnumerable<IDiagnosticValidator>.

If only one implementation should be used, register only the required one.

codereviewbot-ai · 2026-01-31T15:27:15Z

src/PoVicTranslate.Web/Services/ILyricsService.cs

    /// <summary>
    /// Gets lyrics for a specific song, limited to 200 words.
    /// </summary>
-    Task<string?> GetLyricsAsync(string songFileName);
+    Task<string?> GetLyricsAsync(string songTitle);



Ambiguity in Error Handling

The interface methods GetLyricsAsync and GetRandomLyricsAsync return nullable types, but do not specify how errors (e.g., song not found, IO exceptions) should be handled or propagated. This can lead to inconsistent error handling across implementations. Consider defining a consistent error handling strategy, such as returning a result type that encapsulates both data and error information, or documenting expected exception behavior.

codereviewbot-ai · 2026-01-31T15:27:15Z

src/PoVicTranslate.Web/Services/ILyricsService.cs

    /// <summary>
    /// Gets lyrics for a specific song, limited to 200 words.
    /// </summary>
-    Task<string?> GetLyricsAsync(string songFileName);
+    Task<string?> GetLyricsAsync(string songTitle);



Unclear Enforcement of 200-Word Limit

The summary for GetLyricsAsync states that lyrics are limited to 200 words, but the interface does not specify how this limit is enforced or whether it is the responsibility of the implementation. This ambiguity may result in inconsistent behavior. It is recommended to clarify in the interface contract whether the limit is enforced by the service or expected from the data source, and to document the expected behavior.

codereviewbot-ai · 2026-01-31T15:27:15Z

src/PoVicTranslate.Web/Services/LyricsService.cs

+            _lyricsCache = JsonSerializer.Deserialize<LyricsCollection>(jsonContent, _jsonOptions);

            if (_lyricsCache is null)
            {


The deserialization of the lyrics collection does not handle exceptions that may occur if the JSON is malformed or unreadable. This could result in an unhandled exception and application crash.

Recommendation:
Wrap the deserialization in a try-catch block and log the exception, returning a controlled error or null value as appropriate. For example:

try { _lyricsCache = JsonSerializer.Deserialize<LyricsCollection>(jsonContent, _jsonOptions); } catch (JsonException ex) { _logger.LogError(ex, "Failed to deserialize lyrics collection JSON"); throw new InvalidOperationException("Failed to deserialize lyrics collection", ex); }

Copilot

Pull request overview

This PR fixes lyrics loading by aligning JSON deserialization with the lyrics-collection.json schema, updates the lyrics API integration test to hit the correct titles endpoint, and changes several DI registrations to Singleton to support caching.

Changes:

Fix lyrics JSON deserialization (map content → Lyrics, enable case-insensitive property matching).
Adjust lyrics lookup to use songTitle directly (avoid stripping dotted titles).
Update integration test endpoint and change multiple service lifetimes to Singleton; update global.json SDK pin.

Reviewed changes

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

Show a summary per file

File	Description
tests/Po.VicTranslate.IntegrationTests/LyricsEndpointTests.cs	Updates the integration test to call `/api/lyrics/titles`.
src/PoVicTranslate.Web/Services/LyricsService.cs	Uses case-insensitive JSON options and changes lookup to compare by `songTitle`.
src/PoVicTranslate.Web/Services/ILyricsService.cs	Renames the API parameter from `songFileName` to `songTitle`.
src/PoVicTranslate.Web/Program.cs	Switches several service registrations from Scoped to Singleton.
src/PoVicTranslate.Web/Models/LyricsCollection.cs	Maps JSON `content` field onto `SongEntry.Lyrics`.
global.json	Pins the repo SDK version to 10.0.100.

Copilot · 2026-01-31T15:31:12Z

global.json

+    "version": "10.0.100",
    "rollForward": "latestPatch",
    "allowPrerelease": false


global.json now pins the SDK to 10.0.100 with rollForward: latestPatch. latestPatch will not roll forward across SDK feature bands (e.g., 10.0.100 -> 10.0.102), so this can break builds on machines/CI that only have 10.0.102 installed (your GitHub Actions workflow installs 10.0.x). Consider either pinning to the feature band used in CI (e.g., 10.0.102) or changing rollForward to latestFeature/latestMinor, or updating CI to install 10.0.100 explicitly.

Copilot · 2026-01-31T15:31:12Z

src/PoVicTranslate.Web/Program.cs

 // Register validation services
 builder.Services.AddSingleton<ISpeechConfigValidator, SpeechConfigValidator>();
-builder.Services.AddScoped<IAudioSynthesisService, AudioSynthesisService>();
+builder.Services.AddSingleton<IAudioSynthesisService, AudioSynthesisService>();


Switching AudioSynthesisService to Singleton introduces shared mutable state across requests (_accessToken/_tokenExpiry in AudioSynthesisService). As written, token refresh is not synchronized, so concurrent calls can race and trigger multiple token fetches (or transiently observe stale values). Either keep this service scoped, or add synchronization around token refresh (e.g., a lock/SemaphoreSlim) or move token caching into IMemoryCache with proper concurrency control.

Suggested change

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

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

Copilot · 2026-01-31T15:31:13Z

src/PoVicTranslate.Web/Services/LyricsService.cs

            var jsonContent = await File.ReadAllTextAsync(_lyricsFilePath);
-            _lyricsCache = JsonSerializer.Deserialize<LyricsCollection>(jsonContent);
+            _lyricsCache = JsonSerializer.Deserialize<LyricsCollection>(jsonContent, _jsonOptions);



The PR fixes JSON deserialization and changes how lyrics are looked up by songTitle, but the integration tests still don’t cover the successful path of /api/lyrics/{songTitle} returning non-empty lyrics (and the 200-word truncation behavior). Adding a test that fetches /api/lyrics/titles, picks a title, then calls /api/lyrics/{title} and asserts a non-empty Lyrics field (and optionally word-count <= 200) would prevent regressions.

Fix lyrics JSON deserialization, service lifetimes, and test URL

6c73e4f

Co-authored-by: punkouter26 <[email protected]>

Copilot AI review requested due to automatic review settings January 31, 2026 15:26

Copilot started reviewing on behalf of punkouter26 January 31, 2026 15:26 View session

codereviewbot-ai bot reviewed Jan 31, 2026

View reviewed changes

Copilot AI reviewed Jan 31, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix lyrics JSON deserialization and service lifetimes #4

Fix lyrics JSON deserialization and service lifetimes #4

Uh oh!

punkouter26 commented Jan 31, 2026

Uh oh!

google-labs-jules bot commented Jan 31, 2026

Uh oh!

codereviewbot-ai bot Jan 31, 2026

Uh oh!

codereviewbot-ai bot Jan 31, 2026

Uh oh!

codereviewbot-ai bot Jan 31, 2026

Uh oh!

codereviewbot-ai bot Jan 31, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 31, 2026

Uh oh!

Copilot AI Jan 31, 2026

Uh oh!

Copilot AI Jan 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

Fix lyrics JSON deserialization and service lifetimes #4

Are you sure you want to change the base?

Fix lyrics JSON deserialization and service lifetimes #4

Uh oh!

Conversation

punkouter26 commented Jan 31, 2026

Uh oh!

google-labs-jules bot commented Jan 31, 2026

Uh oh!

codereviewbot-ai bot Jan 31, 2026

Choose a reason for hiding this comment

Uh oh!

codereviewbot-ai bot Jan 31, 2026

Choose a reason for hiding this comment

Ambiguity in Error Handling

Uh oh!

codereviewbot-ai bot Jan 31, 2026

Choose a reason for hiding this comment

Unclear Enforcement of 200-Word Limit

Uh oh!

codereviewbot-ai bot Jan 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants