Skip to content

feat: Tidal Integration#33

Open
oca-agent wants to merge 2 commits into
mainfrom
feat/tidal-integration
Open

feat: Tidal Integration#33
oca-agent wants to merge 2 commits into
mainfrom
feat/tidal-integration

Conversation

@oca-agent
Copy link
Copy Markdown
Collaborator

Implements Tidal integration for SMoC (Issue #28). Adds metadata search, browsing, and lossless stream playback via Device Authorization flow.

@oca-agent oca-agent requested a review from mrazza May 6, 2026 03:18
@oca-agent
Copy link
Copy Markdown
Collaborator Author

Senior Code Review Feedback 🔍

A very solid start on the Tidal integration! Implementing the device authorization flow, metadata lookup, and parsing playback info manifests is excellent.

Before we merge this, there are some critical UX and robustness concerns that must be addressed:

1. Terminal GUI UX (Background Authorization Hang)

In TidalStreamingClient.AuthorizeDeviceAsync, you output instructions using:

Logging.Information($"Please visit {authData.VerificationUriComplete} or go to {authData.VerificationUri} and enter code: {authData.UserCode}");
  • The Concern: SMoC is a Terminal.Gui console application. Standard logging and stderr are typically redirected to a file or hidden entirely behind the fullscreen GUI layout.
  • If a user tries to authenticate, they will never see this log message on their screen. The application will appear to hang or freeze in the background while it polls for authorization, with the user completely unaware they need to visit a web page and type a code.
  • The Fix: We need to wire this up to the UI. Instead of purely logging, trigger a modal dialog box or prompt within the terminal UI (e.g., a MessageBox.Query or custom dialog) displaying the verification URL and code, and giving the user a "Cancel" button to abort polling.

2. Unimplemented Token Refresh Retry

In GetAsync, you have a placeholder TODO:

if (response.StatusCode == System.Net.HttpStatusCode.Unauthorized) {
    // Refresh token and retry once
    // TODO: Implement refresh logic
}
  • If a token expires during an active session or right at the boundary, this will throw an exception and crash the streaming view.
  • The Fix: Please implement the token refresh retry here. You can call RefreshTokenAsync(cancellationToken) and then replay the request once.

3. Use of Reflection in Test Setup

In TidalStreamingClient.CreateForTesting, reflection is used to set the private field _httpClient:

var httpClientField = typeof(TidalStreamingClient).GetField("_httpClient", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance);
httpClientField?.SetValue(client, httpClient);
  • The Suggestion: Reflection is fragile and can break if the field is renamed. Instead, define an internal constructor for testing:
    internal TidalStreamingClient(HttpClient httpClient, ICacheService? songCacheService = null, ICacheService? albumArtCacheService = null) : this(songCacheService, albumArtCacheService) {
        _httpClient = httpClient;
    }
    You can make this constructor visible to your test assembly by adding the following attribute in AssemblyInfo.cs or main assembly level:
    [assembly: InternalsVisibleTo("smoc.Tests")]

4. Fragile URL Parsing

In GetPlaylistSongsFromUrlAsync:

var id = url.Split("/track/").Last().Split('/').First();
  • If the URL doesn't contain a trailing slash or has query parameters, this string manipulation can throw an IndexOutOfRangeException or fail.
  • The Fix: Consider using regex or System.Uri to reliably parse path segments.

Excellent progress! Let's address these, and this will be ready for a final review.

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