feat: Add application update functionality with Velopack #3

undead2146 · 2025-12-14T18:39:50Z

Summary by CodeRabbit

New Features
- Added update channel selection (Stable, Prerelease, Artifacts).
- Users can now subscribe to and install pull request build artifacts.
- GitHub Personal Access Token management for accessing test builds.
UI/UX Improvements
- Redesigned update notification window with improved dark theme.
- Settings now include comprehensive update and artifact management options.
- Enhanced update status display and PR subscription controls.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai

Actionable comments posted: 2

♻️ Duplicate comments (32)

GenHub/GenHub/Features/GitHub/ViewModels/GitHubTokenDialogViewModel.cs (4)
152-169: Scope check uses substring matching.

The Contains("repo") check could match unintended scope names. A previous review flagged this issue with a suggested fix to split and match exact scope names.

192-198: Exception message exposed to user.

A previous review flagged that ex.Message is exposed directly in the UI on line 194, which could leak sensitive details. Consider using a generic error message.

232-238: Exception message exposed to user.

A previous review flagged that ex.Message is exposed directly in the UI on line 234, which could leak sensitive details.

143-145: Do not dispose HttpClient created from IHttpClientFactory.

HttpClients created from IHttpClientFactory should NOT be disposed, as the factory manages their lifecycle and connection pooling. Disposing them can lead to socket exhaustion.
-        using var client = _httpClientFactory.CreateClient();
+        var client = _httpClientFactory.CreateClient();
GenHub/GenHub/Features/GameProfiles/Services/ProfileContentLoader.cs (1)

231-251: Incomplete property cloning may cause data loss.

This issue was previously flagged. The method only copies a subset of ContentDisplayItem properties (Id, ManifestId, DisplayName, Description, Version, ContentType, GameType, InstallationType, Publisher, SourceId, GameClientId, IsEnabled). Properties like ReleaseDate, FileSize, IconUrl, Tags, Metadata, Dependencies, IsInstalled, IsAvailable, and others are not copied.
GenHub/GenHub.Core/Models/AppUpdate/ArtifactUpdateInfo.cs (2)
22-22: Consider using DateTimeOffset for UTC clarity.

GitHub timestamps are UTC; storing as DateTimeOffset avoids DateTimeKind ambiguity when serializing/deserializing.
-    DateTime createdAt)
+    DateTimeOffset createdAt)
72-84: Guard DisplayVersion against empty GitHash.

If GitHash is empty or whitespace, the output will include awkward trailing (). Consider omitting the hash portion when empty.
 public string DisplayVersion
 {
     get
     {
+        var hashPart = string.IsNullOrWhiteSpace(GitHash) ? string.Empty : $" ({GitHash})";
         if (PullRequestNumber.HasValue)
         {
             var baseVersion = Version.Split('+')[0];
-            return $"v{baseVersion}+#{PullRequestNumber} ({GitHash})";
+            return $"v{baseVersion}+#{PullRequestNumber}{hashPart}";
         }
 
-        return $"v{Version} ({GitHash})";
+        return $"v{Version}{hashPart}";
     }
 }
GenHub/GenHub/Features/AppUpdate/Services/SimpleHttpServer.cs (4)
34-43: Add port validation before constructing the listener.

The constructor should validate the port range before using it. Invalid ports will cause hard-to-diagnose HttpListenerExceptions.
 public SimpleHttpServer(string nupkgPath, string releasesPath, int port, ILogger logger)
 {
+    if (port is < 1 or > 65535)
+        throw new ArgumentOutOfRangeException(nameof(port), "Port must be 1..65535.");
     _nupkgPath = nupkgPath ?? throw new ArgumentNullException(nameof(nupkgPath));
64-84: Stop() leaves server non-restartable and leaks CTS.

Stop() never resets _serverTask or disposes _cts, so calling Start() after Stop() is a no-op. The server becomes single-use.
     _logger.LogInformation("HTTP server stopped");
+
+    _serverTask = null;
+    _cts.Dispose();
+    _cts = null;
 }
114-172: Ensure HTTP response is always closed via finally block.

If an exception occurs before Line 166, the response remains open, causing clients to hang and resource exhaustion.
 private async Task ProcessRequestAsync(HttpListenerContext context)
 {
+    var response = context.Response;
     try
     {
         var request = context.Request;
-        var response = context.Response;
         // ... existing logic ...
-        response.Close();
     }
     catch (Exception ex)
     {
         _logger.LogError(ex, "Error processing HTTP request");
     }
+    finally
+    {
+        try { response.Close(); } catch { /* best-effort */ }
+    }
 }
94-112: Fire-and-forget task may swallow exceptions.

The discarded task on Line 101 means exceptions in ProcessRequestAsync go unobserved. Consider adding a continuation to log failures.
-                _ = Task.Run(() => ProcessRequestAsync(context), cancellationToken);
+                var requestTask = Task.Run(() => ProcessRequestAsync(context), cancellationToken);
+                _ = requestTask.ContinueWith(t =>
+                {
+                    if (t.IsFaulted && t.Exception != null)
+                        _logger.LogError(t.Exception, "Unhandled error in request processing");
+                }, TaskScheduler.Default);
.github/workflows/ci.yml (3)
88-94: Update actions/cache from v3 to v4.

Both NuGet cache steps use actions/cache@v3 which is deprecated. The workflow already uses v4 for other actions (checkout, setup-dotnet, upload-artifact).
       - name: Cache NuGet Packages
-        uses: actions/cache@v3
+        uses: actions/cache@v4
         with:
           path: ~/.nuget/packages
Apply to both Windows (Line 89) and Linux (Line 247) cache steps.

Also applies to: 246-252

278-280: Export CHANNEL to GitHub Output on Linux.

The Linux Extract Build Info step computes CHANNEL but doesn't export it to $GITHUB_OUTPUT. Downstream steps referencing ${{ steps.buildinfo.outputs.CHANNEL }} will receive an empty value, causing assembly metadata to fall back to "Dev" instead of "CI" or "PR".
           echo "SHORT_HASH=$SHORT_HASH" >> $GITHUB_OUTPUT
           echo "PR_NUMBER=$PR_NUMBER" >> $GITHUB_OUTPUT
           echo "VERSION=$VERSION" >> $GITHUB_OUTPUT
+          echo "CHANNEL=$CHANNEL" >> $GITHUB_OUTPUT
386-388: Parameterize release tag using computed build version.

Line 386 hardcodes the release tag as v0.0.${{ github.run_number }}, but the actual version is computed in the build jobs. If version computation changes, the tag will diverge.

Add outputs to the build-windows job and reference them:
# In build-windows job header:
    outputs:
      version: ${{ steps.buildinfo.outputs.VERSION }}

# In release step:
          tag_name: v${{ needs.build-windows.outputs.version }}
          name: GenHub Alpha ${{ needs.build-windows.outputs.version }}
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/Services/VelopackUpdateManagerTests.cs (1)
164-169: Log assertion still uses permissive OR logic.

This was flagged in a previous review but remains unaddressed. The assertion accepts log messages containing "Velopack" OR "Update", which is too broad. The actual warning message is "Velopack UpdateManager not available (running from Debug)", so requiring both terms would be more precise.
                 It.Is<It.IsAnyType>((v, t) =>
-                    (v.ToString() ?? string.Empty).Contains("Velopack") ||
-                    (v.ToString() ?? string.Empty).Contains("Update")),
+                    (v.ToString() ?? string.Empty).Contains("Velopack") &&
+                    (v.ToString() ?? string.Empty).Contains("UpdateManager")),
GenHub/GenHub/Features/Settings/Views/SettingsView.axaml (3)
222-223: Formatting inconsistency persists.

There's still a double space before the opening parenthesis and a missing space after the comma: "operation (10-3600 seconds,default:".
-            <TextBlock Text="Timeout for each download operation  (10-3600 seconds,default: 600 seconds)"
+            <TextBlock Text="Timeout for each download operation (10-3600 seconds, default: 600 seconds)"
494-500: PatStatusColor binding requires a Brush type or value converter.

Ellipse.Fill expects a Brush, but per prior review, PatStatusColor returns a hex string. This will cause a runtime binding error.

Either modify the ViewModel to return SolidColorBrush:
public SolidColorBrush PatStatusColor => new SolidColorBrush(Color.Parse(patColorHex));
Or add a StringToBrushConverter and use it in the binding:
Fill="{Binding PatStatusColor, Converter={StaticResource StringToBrushConverter}}"
570-572: Integer negation binding may not work as intended.

IsVisible="{Binding !AvailableArtifacts.Count}" attempts to negate an integer, which may produce unexpected results or binding errors in Avalonia.

Add a computed boolean property in the ViewModel:
public bool HasNoArtifacts => AvailableArtifacts?.Count == 0;
Then bind:
-           IsVisible="{Binding !AvailableArtifacts.Count}"
+           IsVisible="{Binding HasNoArtifacts}"
GenHub/GenHub/Features/AppUpdate/ViewModels/UpdateNotificationViewModel.cs (5)
540-556: Fire-and-forget SaveAsync() without error handling.

Line 546 uses _ = _userSettingsService.SaveAsync() which discards the task. If saving fails, the error is silently lost and the user won't know their dismissal wasn't persisted.

Consider logging failures:
         if (!string.IsNullOrEmpty(LatestVersion))
         {
             _userSettingsService.Update(s => s.DismissedUpdateVersion = LatestVersion);
-            _ = _userSettingsService.SaveAsync();
+            _ = _userSettingsService.SaveAsync().ContinueWith(t =>
+            {
+                if (t.IsFaulted)
+                    _logger.LogError(t.Exception, "Failed to save dismissed update version");
+            }, TaskScheduler.Default);
             _logger.LogInformation("Dismissed update version {Version}", LatestVersion);
         }
143-167: Fire-and-forget async call lacks top-level exception handling.

Line 143 uses _ = InitializeAsync() which discards the task. The method at lines 149-167 has no surrounding try-catch, so any unhandled exceptions from LoadPullRequestsAsync or CheckForUpdatesAsync will be unobserved.

Wrap the method body in try-catch:
 private async Task InitializeAsync()
 {
+    try
+    {
         // Load subscribed PR from settings
         var settings = _userSettingsService.Get();
         if (settings.SubscribedPrNumber.HasValue)
         {
             _velopackUpdateManager.SubscribedPrNumber = settings.SubscribedPrNumber;
             _logger.LogInformation("Loaded subscribed PR #{PrNumber} from settings", settings.SubscribedPrNumber);
         }
 
         // Load PRs FIRST so SubscribedPr object is populated before update check
         if (HasPat)
         {
             await LoadPullRequestsAsync();
         }
 
         // Now check for updates - SubscribedPr will be properly populated
         await CheckForUpdatesAsync();
+    }
+    catch (Exception ex)
+    {
+        _logger.LogError(ex, "Initialization failed");
+        HasError = true;
+        ErrorMessage = $"Initialization failed: {ex.Message}";
+        StatusMessage = "Initialization failed";
+    }
 }
601-650: Marshal AvailablePullRequests.Clear() to UI thread.

Line 609 calls AvailablePullRequests.Clear() off the UI thread, while the subsequent Add operations (lines 617-623) are correctly marshaled. The Clear should also run on the UI thread to prevent collection corruption.

Apply this fix:
         IsLoadingPullRequests = true;
-        AvailablePullRequests.Clear();
+        await Dispatcher.UIThread.InvokeAsync(() => AvailablePullRequests.Clear());
 
         try
         {
657-672: Fire-and-forget SaveAsync() in SubscribeToPr.

Line 665 discards the SaveAsync() task. If saving fails, the subscription won't be persisted but the user won't know.

Apply the same fix pattern as in other comments - add a ContinueWith to log failures.

684-696: Fire-and-forget SaveAsync() in UnsubscribeFromPr.

Line 693 discards the SaveAsync() task, same issue as SubscribeToPr.
GenHub/GenHub/Features/Settings/ViewModels/SettingsViewModel.cs (4)
698-736: LoadPatStatusAsync treats "token exists" as "token valid" without validation.

Lines 709-713 set IsPatValid = true based solely on HasToken(), without verifying the token against GitHub. An expired or revoked PAT will still show as valid. Consider splitting "configured" vs "validated" state, or performing an explicit validation check.

742-811: PAT validation logic bug: token saved before validation, and validation always succeeds.

Two critical issues remain (flagged in past reviews but not fixed):

Line 768: Token is saved to storage BEFORE validation completes at line 777. If validation fails, the invalid token remains stored.

Line 778: The condition artifact != null || _gitHubTokenStorage.HasToken() will ALWAYS be true after line 768's save, even for invalid/unauthorized tokens. This means any PAT will show "validated successfully" regardless of actual validity.

These issues mean users can save invalid PATs that appear validated but won't work.

Apply this fix:
         try
         {
-            // Save temporarily to test
             using var secureString = new System.Security.SecureString();
             foreach (char c in GitHubPatInput)
             {
                 secureString.AppendChar(c);
             }
 
-            await _gitHubTokenStorage.SaveTokenAsync(secureString);
-
             // Try to check for artifacts to validate the PAT
             if (_updateManager != null)
             {
-                // Validate first by making a test call (similar to GitHubTokenDialogViewModel)
-                // Only save after validation succeeds
                 try
                 {
                     var artifact = await _updateManager.CheckForArtifactUpdatesAsync();
-                    if (artifact != null || _gitHubTokenStorage.HasToken())
+                    if (artifact != null)
                     {
+                        // Only save after successful validation
+                        await _gitHubTokenStorage.SaveTokenAsync(secureString);
                         PatStatusMessage = "PAT validated successfully ✓";
                         IsPatValid = true;
                         HasGitHubPat = true;
                         GitHubPatInput = string.Empty;
                         return;
                     }
+                    // No artifacts found - cannot validate scopes
+                    PatStatusMessage = "PAT saved, but no artifacts found (cannot validate scopes)";
+                    IsPatValid = false;
+                    HasGitHubPat = false;
                 }
                 catch
                 {
-                    // Rollback on validation failure
-                    await _gitHubTokenStorage.DeleteTokenAsync();
+                    // Don't need rollback since we never saved
                     throw;
                 }
             }
867-900: Marshal AvailableArtifacts mutations to the UI thread.

Lines 876 and 883 mutate the ObservableCollection AvailableArtifacts without marshaling to the UI thread. Since this is an async method that can resume off the UI thread after await, these mutations can crash Avalonia bindings.

Apply this fix:
+using Avalonia.Threading;
         IsLoadingArtifacts = true;
-        AvailableArtifacts.Clear();
+        await Dispatcher.UIThread.InvokeAsync(() => AvailableArtifacts.Clear());
 
         try
         {
             var artifact = await _updateManager.CheckForArtifactUpdatesAsync();
             if (artifact != null)
             {
-                AvailableArtifacts.Add(artifact);
+                await Dispatcher.UIThread.InvokeAsync(() => AvailableArtifacts.Add(artifact));
                 PatStatusMessage = $"Found {AvailableArtifacts.Count} artifact(s)";
             }
905-927: Add SaveAsync() call to persist the update channel setting.

Line 916 calls Update() which only modifies in-memory state. Per the IUserSettingsService interface documentation, SaveAsync() must be called to persist changes to disk. The channel change will be lost on restart.

Apply this fix:
         // Persist setting
         _userSettingsService.Update(s => s.UpdateChannel = value);
+        _ = _userSettingsService.SaveAsync().ContinueWith(t =>
+        {
+            if (t.IsFaulted)
+                _logger.LogError(t.Exception, "Failed to save update channel setting");
+        }, TaskScheduler.Default);
 
         // If switching to Artifacts, load available artifacts
GenHub/GenHub/Features/AppUpdate/Services/VelopackUpdateManager.cs (5)
62-79: Remove debug logging from property getters.

Lines 66 and 76 log on every property access. UI bindings will poll these properties frequently, flooding logs and adding overhead.

Apply this fix:
 public bool HasUpdateAvailableFromGitHub
 {
     get
     {
-        logger.LogDebug("HasUpdateAvailableFromGitHub property accessed: {Value}", _hasUpdateFromGitHub);
         return _hasUpdateFromGitHub;
     }
 }
 
 public string? LatestVersionFromGitHub
 {
     get
     {
-        logger.LogDebug("LatestVersionFromGitHub property accessed: '{Value}'", _latestVersionFromGitHub ?? "NULL");
         return _latestVersionFromGitHub;
     }
 }
187-298: SecureString token not disposed after use.

Line 203 loads a SecureString but it's not wrapped in a using statement. The token should be disposed promptly to clear sensitive data from memory.

Apply this fix:
         try
         {
-            var token = await gitHubTokenStorage.LoadTokenAsync();
+            using var token = await gitHubTokenStorage.LoadTokenAsync();
             if (token == null)
             {
                 logger.LogWarning("Failed to load GitHub PAT");
                 return null;
             }
 
             using var client = CreateConfiguredHttpClient(token);
301-513: Two issues: SecureString not disposed, and N+1 GitHub API pattern remains.

SecureString disposal (line 319): The token from LoadTokenAsync isn't disposed.

N+1 API pattern (lines 376-468): Despite past review comment claiming this was addressed, the code still fetches workflow runs inside the PR loop (lines 376-377). With 30 PRs, this makes 30+ API calls, quickly exhausting rate limits.

Fix 1: Add using for the token:
-            var token = await gitHubTokenStorage.LoadTokenAsync();
+            using var token = await gitHubTokenStorage.LoadTokenAsync();
Fix 2: Move the runs fetch outside the loop. Fetch runs once, group by PR number, then look up in the loop:
// Fetch all recent runs ONCE before the PR loop
var runsUrl = $"https://api.github.com/repos/{owner}/{repo}/actions/runs?status=success&per_page=100";
var runsResponse = await client.GetAsync(runsUrl, cancellationToken);

var runsByPr = new Dictionary<int, JsonElement>();
if (runsResponse.IsSuccessStatusCode)
{
    var runsJson = await runsResponse.Content.ReadAsStringAsync(cancellationToken);
    var runsData = JsonSerializer.Deserialize<JsonElement>(runsJson);
    
    if (runsData.TryGetProperty("workflow_runs", out var runs))
    {
        foreach (var run in runs.EnumerateArray())
        {
            if (run.TryGetProperty("pull_requests", out var runPrs) && runPrs.GetArrayLength() > 0)
            {
                var prNum = runPrs[0].GetProperty("number").GetInt32();
                if (!runsByPr.ContainsKey(prNum))
                    runsByPr[prNum] = run;
            }
        }
    }
}

foreach (var pr in prsData.EnumerateArray())
{
    var prNumber = pr.GetProperty("number").GetInt32();
    // ... existing property extraction ...
    
    // Look up pre-fetched run for this PR
    ArtifactUpdateInfo? latestArtifact = null;
    if (runsByPr.TryGetValue(prNumber, out var run))
    {
        // ... existing artifact fetch logic using 'run' ...
    }
}
624-831: SecureString not disposed in InstallPrArtifactAsync.

Line 643 loads a SecureString but doesn't dispose it. Add using:
-            var token = await gitHubTokenStorage.LoadTokenAsync();
+            using var token = await gitHubTokenStorage.LoadTokenAsync();
868-875: TOCTOU race in FindAvailablePort.

The port is released immediately after listener.Stop(), creating a window where another process could claim it before the HTTP server binds. This is a minor edge case but could cause intermittent failures.

Consider implementing retry logic around the HTTP server bind, or document this limitation.

🧹 Nitpick comments (3)

GenHub/GenHub/Features/GitHub/ViewModels/GitHubTokenDialogViewModel.cs (2)
96-99: Consider completing the dispose pattern.

The Dispose method should call GC.SuppressFinalize(this) to prevent the finalizer from running if the object is already disposed. This is the standard IDisposable pattern.
 public void Dispose()
 {
     _secureToken?.Dispose();
+    GC.SuppressFinalize(this);
 }
147-147: Consider adding a timeout for the HTTP request.

The API call has no timeout configured, which could cause the UI to hang indefinitely if the GitHub API is unresponsive. Consider using HttpClient.Timeout or a CancellationTokenSource with a timeout.
+        using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30));
-        var response = await client.GetAsync("https://api.github.com/user");
+        var response = await client.GetAsync("https://api.github.com/user", cts.Token);
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/ViewModels/UpdateNotificationViewModelTests.cs (1)
25-31: LGTM! Tests correctly accommodate the new constructor dependency.

The mock IUserSettingsService setup is appropriate and follows the pattern used elsewhere in the test suite.

Consider extracting the repeated mock setup to a shared field or helper to reduce duplication:
private static Mock<IUserSettingsService> CreateMockUserSettingsService()
{
    var mock = new Mock<IUserSettingsService>();
    mock.Setup(x => x.Get()).Returns(new UserSettings());
    return mock;
}
Also applies to: 45-51, 65-71

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e867ce6 and ede51ab.

📒 Files selected for processing (30)

.github/workflows/ci.yml (5 hunks)
GenHub/Directory.Build.props (1 hunks)
GenHub/GenHub.Core/Constants/AppConstants.cs (5 hunks)
GenHub/GenHub.Core/Constants/BuildInfoConstants.cs (1 hunks)
GenHub/GenHub.Core/Constants/GitHubConstants.cs (1 hunks)
GenHub/GenHub.Core/Helpers/FileSizeFormatter.cs (1 hunks)
GenHub/GenHub.Core/Models/AppUpdate/ArtifactUpdateInfo.cs (1 hunks)
GenHub/GenHub.Core/Models/AppUpdate/PullRequestInfo.cs (1 hunks)
GenHub/GenHub.Core/Models/Common/UserSettings.cs (2 hunks)
GenHub/GenHub.Core/Models/Enums/UpdateChannel.cs (1 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Constants/AppConstantsTests.cs (1 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/Services/VelopackUpdateManagerTests.cs (12 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/ViewModels/UpdateNotificationViewModelTests.cs (4 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameProfiles/GameProcessManagerTests.cs (1 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/GameProfileModuleTests.cs (5 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/SharedViewModelModuleTests.cs (3 hunks)
GenHub/GenHub/Common/ViewModels/MainViewModel.cs (1 hunks)
GenHub/GenHub/Features/AppUpdate/Interfaces/IVelopackUpdateManager.cs (3 hunks)
GenHub/GenHub/Features/AppUpdate/Services/SimpleHttpServer.cs (1 hunks)
GenHub/GenHub/Features/AppUpdate/Services/VelopackUpdateManager.cs (7 hunks)
GenHub/GenHub/Features/AppUpdate/ViewModels/UpdateNotificationViewModel.cs (9 hunks)
GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationView.axaml (5 hunks)
GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationWindow.axaml (4 hunks)
GenHub/GenHub/Features/GameProfiles/Services/ProfileContentLoader.cs (5 hunks)
GenHub/GenHub/Features/GitHub/ViewModels/GitHubTokenDialogViewModel.cs (1 hunks)
GenHub/GenHub/Features/GitHub/Views/GitHubTokenDialogView.axaml (1 hunks)
GenHub/GenHub/Features/GitHub/Views/GitHubTokenDialogView.axaml.cs (1 hunks)
GenHub/GenHub/Features/Settings/ViewModels/SettingsViewModel.cs (8 hunks)
GenHub/GenHub/Features/Settings/Views/SettingsView.axaml (18 hunks)
GenHub/GenHub/Features/Settings/Views/SettingsView.axaml.cs (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (9)

GenHub/GenHub.Core/Helpers/FileSizeFormatter.cs
GenHub/GenHub/Common/ViewModels/MainViewModel.cs
GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/GameProfileModuleTests.cs
GenHub/GenHub.Core/Constants/BuildInfoConstants.cs
GenHub/GenHub/Features/GitHub/Views/GitHubTokenDialogView.axaml
GenHub/GenHub/Features/GitHub/Views/GitHubTokenDialogView.axaml.cs
GenHub/GenHub/Features/Settings/Views/SettingsView.axaml.cs
GenHub/GenHub.Core/Models/AppUpdate/PullRequestInfo.cs
GenHub/GenHub.Core/Constants/GitHubConstants.cs

🧰 Additional context used

🧬 Code graph analysis (7)

GenHub/GenHub/Features/AppUpdate/Services/SimpleHttpServer.cs (1)

GenHub/GenHub/Infrastructure/Extensions/LoggerExtensions.cs (1)

LogError (46-51)

GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/ViewModels/UpdateNotificationViewModelTests.cs (3)

GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/SharedViewModelModuleTests.cs (1)

IUserSettingsService (161-172)

GenHub/GenHub.Core/Models/Common/UserSettings.cs (1)

UserSettings (7-148)

GenHub/GenHub/Features/AppUpdate/ViewModels/UpdateNotificationViewModel.cs (2)

UpdateNotificationViewModel (25-697)

UpdateNotificationViewModel (123-144)

GenHub/GenHub.Core/Models/Common/UserSettings.cs (1)

GenHub/GenHub.Core/Interfaces/Common/IConfigurationProviderService.cs (3)

List (114-114)

List (120-120)

CasConfiguration (132-132)

GenHub/GenHub/Features/GameProfiles/Services/ProfileContentLoader.cs (6)

GenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileSettingsViewModel.cs (7)

ObservableCollection (1279-1281)

ContentDisplayItem (1257-1272)

Task (242-303)

Task (310-426)

Task (561-596)

Task (603-631)

Task (1247-1250)

GenHub/GenHub.Core/Models/Content/ContentDisplayItem.cs (1)

ContentDisplayItem (11-234)

GenHub/GenHub.Core/Interfaces/Content/IContentDisplayFormatter.cs (2)

ContentDisplayItem (22-22)

ContentDisplayItem (32-36)

GenHub/GenHub.Core/Models/Manifest/ManifestId.cs (1)

ManifestId (53-61)

GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/Extensions/LoggerExtensionsTests.cs (1)

IsEnabled (144-144)

GenHub/GenHub.Core/Interfaces/GameProfiles/IProfileContentLoader.cs (2)

Task (24-24)

Task (32-32)

GenHub/GenHub.Tests/GenHub.Tests.Core/Constants/AppConstantsTests.cs (1)

GenHub/GenHub.Core/Constants/AppConstants.cs (1)

AppConstants (10-137)

GenHub/GenHub/Features/AppUpdate/ViewModels/UpdateNotificationViewModel.cs (5)

GenHub/GenHub/Features/AppUpdate/Interfaces/IVelopackUpdateManager.cs (5)

Task (21-21)

Task (29-29)

Task (37-37)

Task (46-46)

Task (109-109)

GenHub/GenHub/Features/AppUpdate/Services/VelopackUpdateManager.cs (5)

Task (93-184)

Task (187-298)

Task (301-513)

Task (516-560)

Task (624-831)

GenHub/GenHub.Core/Constants/AppConstants.cs (1)

AppConstants (10-137)

GenHub/GenHub/Infrastructure/Extensions/LoggerExtensions.cs (1)

LogError (46-51)

GenHub/GenHub/Common/Services/UserSettingsService.cs (1)

Update (79-102)

GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/Services/VelopackUpdateManagerTests.cs (2)

GenHub/GenHub.Core/Models/Common/UserSettings.cs (1)

UserSettings (7-148)

GenHub/GenHub/Features/AppUpdate/Interfaces/IVelopackUpdateManager.cs (5)

Task (21-21)

Task (29-29)

Task (37-37)

Task (46-46)

Task (109-109)

🪛 actionlint (1.7.9)

.github/workflows/ci.yml

89-89: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

247-247: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

GitHub Check: Build Windows
GitHub Check: Build Linux

🔇 Additional comments (31)

GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/SharedViewModelModuleTests.cs (3)

104-104: Good improvement: consolidated AddCasServices() registration.

The earlier placement and removal of the duplicate registration improves clarity and ensures proper dependency resolution order.

115-117: Good pattern: platform-specific mocks after module registrations.

Registering the IShortcutService mock after module registrations prevents it from being overridden and makes the dependency structure clearer.

119-121: Previous duplicate registration issue resolved.

The duplicate IGitHubTokenStorage registration flagged in the previous review has been successfully addressed. Only one registration now exists, properly placed after module registrations to prevent override.

GenHub/GenHub/Features/GitHub/ViewModels/GitHubTokenDialogViewModel.cs (3)

104-115: LGTM!

The OpenPatCreation method correctly uses Process.Start with UseShellExecute = true for cross-platform URL opening, with proper exception handling and logging.

244-248: LGTM!

Simple and clean cancel implementation.

250-264: LGTM!

The GetPlainToken method correctly handles SecureString extraction with proper memory cleanup using Marshal.ZeroFreeGlobalAllocUnicode in the finally block.

GenHub/GenHub/Features/GameProfiles/Services/ProfileContentLoader.cs (1)

495-495: Good improvement: named tuple elements enhance readability.

Adding named elements (Installation and Client) to the tuple return type makes the method signature more self-documenting and improves code clarity.

GenHub/GenHub.Tests/GenHub.Tests.Core/Constants/AppConstantsTests.cs (1)

24-31: LGTM! Version assertions align with the new alpha versioning scheme.

The test expectations correctly verify the 0.0.x format and the DisplayVersion "v" prefix, consistent with the versioning changes in AppConstants.cs and Directory.Build.props.

GenHub/GenHub.Core/Models/Enums/UpdateChannel.cs (1)

1-22: LGTM! Clean enum definition with clear documentation.

The UpdateChannel enum provides a well-structured representation of update channels with appropriate XML documentation explaining the purpose and requirements of each option.

GenHub/GenHub.Core/Models/Common/UserSettings.cs (2)

98-111: LGTM! New update-related properties are well-defined.

The three new properties (UpdateChannel, SubscribedPrNumber, DismissedUpdateVersion) are properly documented and follow the existing patterns in the class. The default value of UpdateChannel.Prerelease is a sensible choice for an alpha-stage application.

138-140: Clone method correctly copies the new properties.

The new properties are properly included in the Clone() method. Since UpdateChannel is an enum (value type), SubscribedPrNumber is a nullable value type, and DismissedUpdateVersion is a string (immutable), shallow copying is sufficient.

GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationWindow.axaml (1)

9-9: LGTM! Simplified icon implementation.

Replacing the Path-based icon with an Image element referencing the actual PNG asset is cleaner and more maintainable. The consistent use of /Assets/Icons/generalshub-icon.png for both the window icon and title bar icon ensures visual consistency.

Also applies to: 50-58

GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/Services/VelopackUpdateManagerTests.cs (1)

176-189: LGTM!

The new test properly validates that CheckForArtifactUpdatesAsync returns null and HasArtifactUpdateAvailable is false when no PAT is configured. The explicit setup on line 180 duplicates the constructor default but clarifies the test precondition, which is acceptable.

GenHub/GenHub/Features/Settings/Views/SettingsView.axaml (1)

452-483: LGTM!

The new Updates section is well-structured with appropriate bindings for version display, update channel selection, and the "Manage Updates & PRs" button. The use of XML entities (&) for ampersands is correct.

GenHub/GenHub.Core/Constants/AppConstants.cs (2)

76-94: LGTM!

The FullDisplayVersion property correctly handles three distinct build scenarios (local, CI, PR) with appropriate formatting. The conditional logic is clear and produces meaningful version strings for each context.

129-136: LGTM!

The GetAssemblyMetadata helper is a clean, reusable implementation for extracting assembly metadata attributes. Null-safety is properly handled.

GenHub/GenHub/Features/AppUpdate/Interfaces/IVelopackUpdateManager.cs (2)

23-37: LGTM!

The new artifact-related methods (CheckForArtifactUpdatesAsync, GetOpenPullRequestsAsync) follow consistent async patterns with the existing interface. Documentation clearly specifies the PAT requirement for these operations.

76-100: LGTM!

The new properties appropriately expose artifact update state and PR subscription functionality. The getter/setter design for CurrentChannel and SubscribedPrNumber allows configuration, while HasArtifactUpdateAvailable, LatestArtifactUpdate, and IsPrMergedOrClosed are read-only state indicators.

GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationView.axaml (3)

38-46: LGTM!

The refactored layout correctly places all content sections within a ScrollViewer containing a StackPanel. This resolves the previous Grid.Row overlap issue—multiple visible sections now stack vertically rather than overlapping.

220-238: LGTM!

The Subscribe button correctly uses the $parent[ItemsControl].((vm:UpdateNotificationViewModel)DataContext) pattern to access the ViewModel's SubscribeToPrCommand from within the ItemsControl item template. This is the proper Avalonia approach for command binding in templated items.

245-247: The usage Converter={x:Static ObjectConverters.IsNotNull} without an explicit namespace prefix is correct. Avalonia's official documentation shows this exact pattern as the standard way to access ObjectConverters.IsNotNull, and no namespace prefix is required.

GenHub/Directory.Build.props (3)

3-10: LGTM! Version scheme updated to 0.0.x alpha.

The versioning update is clear and aligns with the PR's alpha development phase.

14-26: LGTM! Build metadata properties for CI integration.

The conditional InformationalVersion correctly appends the Git hash when available, following standard CI/CD practices.

29-43: LGTM! Runtime metadata exposure via assembly attributes.

The AssemblyMetadataAttribute approach is the standard pattern for exposing build-time values to runtime code.

GenHub/GenHub/Features/Settings/ViewModels/SettingsViewModel.cs (3)

817-843: LGTM! PAT deletion properly cleans up state.

The method correctly clears the token, resets UI state, clears artifacts, and switches away from Artifacts channel if needed.

849-861: LGTM! Update window creation.

Straightforward window instantiation with proper error handling.

433-466: LGTM! Update channel properly persisted in SaveSettings.

Line 442 correctly includes the update channel in the settings save flow, which properly calls SaveAsync() at line 466.

GenHub/GenHub/Features/AppUpdate/ViewModels/UpdateNotificationViewModel.cs (2)

260-296: LGTM! PR artifact priority logic.

The prioritization of subscribed PR artifacts over main branch releases is correct, and the dismissed version check properly prevents re-showing dismissed updates.

482-530: LGTM! PR artifact installation flow.

The method properly validates preconditions, reports progress, and handles errors appropriately.

GenHub/GenHub/Features/AppUpdate/Services/VelopackUpdateManager.cs (2)

93-184: LGTM! Update check with proper state reset.

Lines 97-99 correctly reset per-run state to prevent stale UI, addressing previous review feedback.

882-897: LGTM! Version extraction from artifact names.

The method correctly handles both Windows and Linux platform prefixes.

coderabbitai · 2025-12-16T20:40:44Z

GenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameProfiles/GameProcessManagerTests.cs

        }
    }
-}
+}


⚠️ Potential issue | 🟡 Minor

Consider restoring the explicit positive-path test for StartProcessAsync.

The removal of StartProcessAsync_WithValidExecutablePath_ShouldReturnSuccess() eliminates the dedicated test for the success scenario. While three other tests (lines 137, 200, 268) indirectly validate StartProcessAsync by calling it in their setup, an explicit positive-path test provides:

Clear documentation of expected behavior for the success case

Test isolation - failures are easier to diagnose when tests are focused

Better maintainability - if those other tests change their setup, the success path coverage could be inadvertently lost

If the test was removed due to flakiness or another specific reason, please document that rationale. Otherwise, consider restoring it.

🤖 Prompt for AI Agents

In GenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameProfiles/GameProcessManagerTests.cs around line 288, restore an explicit positive-path unit test named StartProcessAsync_WithValidExecutablePath_ShouldReturnSuccess that independently exercises StartProcessAsync: arrange a minimal deterministic setup (mock required dependencies to return success, use a valid temp executable path or a mocked process runner), act by calling StartProcessAsync, and assert that the returned result indicates success and any expected side-effects (e.g., process tracked, status updated). Keep the test isolated (no shared setup from other tests), make mocks deterministic to avoid flakiness, and if this test was removed for flakiness, add a brief inline comment explaining that and how flakiness was addressed.

GenHub/GenHub/Features/Settings/ViewModels/SettingsViewModel.cs

coderabbitai

Actionable comments posted: 2

♻️ Duplicate comments (37)

GenHub/GenHub/Features/GameProfiles/Services/ProfileContentLoader.cs (1)

231-251: Incomplete property cloning may cause data loss.

CloneWithEnabledState only copies a subset of ContentDisplayItem properties. The model definition includes additional properties such as ReleaseDate, FileSize, IconUrl, Tags, Metadata, Dependencies, IsInstalled, and IsAvailable that are not copied. If source items have these properties populated (e.g., when availableGameInstallations is passed from external sources on line 149), those values will be lost in the cloned collection.

Consider implementing a proper clone method on ContentDisplayItem that copies all properties, or at minimum document which properties are intentionally excluded and why.
GenHub/GenHub.Core/Models/AppUpdate/ArtifactUpdateInfo.cs (2)
22-22: Prefer DateTimeOffset for CreatedAt to clarify UTC intent.

GitHub timestamps are in UTC. Using DateTimeOffset instead of DateTime avoids DateTimeKind ambiguity and makes UTC handling explicit.

Apply this diff:
 public record ArtifactUpdateInfo(
     string version,
     string gitHash,
     int? pullRequestNumber,
     long workflowRunId,
     string workflowRunUrl,
     long artifactId,
     string artifactName,
-    DateTime createdAt)
+    DateTimeOffset createdAt)
 {
     // ...
     
     /// <summary>
     /// Gets when the artifact was created.
     /// </summary>
-    public DateTime CreatedAt { get; init; } = createdAt;
+    public DateTimeOffset CreatedAt { get; init; } = createdAt;
Also applies to: 62-62

72-84: Guard DisplayVersion against empty GitHash to avoid odd UI strings.

When GitHash is empty or whitespace, the current implementation produces strings like "v0.0.1 ()" or "v0.0.1+#42 ()", which appear incomplete in the UI.

Apply this diff:
     public string DisplayVersion
     {
         get
         {
+            var hashPart = string.IsNullOrWhiteSpace(GitHash) ? string.Empty : $" ({GitHash})";
             if (PullRequestNumber.HasValue)
             {
                 var baseVersion = Version.Split('+')[0];
-                return $"v{baseVersion}+#{PullRequestNumber} ({GitHash})";
+                return $"v{baseVersion}+#{PullRequestNumber}{hashPart}";
             }

-            return $"v{Version} ({GitHash})";
+            return $"v{Version}{hashPart}";
         }
     }
GenHub/GenHub.Core/Constants/BuildInfoConstants.cs (1)

1-42: Address duplication between BuildInfoConstants and AppConstants.

This class introduces compile-time constants for build metadata (GitShortHash, PullRequestNumber, BuildChannel), but AppConstants already exposes these same properties by reading from assembly metadata at runtime. Maintaining parallel accessors in two separate classes creates a dual source of truth and increases maintenance burden.

Consider consolidating to a single source:

If MSBuild properties populate assembly metadata, AppConstants alone should suffice.

If BuildInfoConstants serves as the backing store, refactor AppConstants to delegate to it rather than using lazy assembly metadata lookup.

Clarify the design intent and eliminate redundancy.
GenHub/GenHub/Features/AppUpdate/Services/SimpleHttpServer.cs (4)
34-43: Validate port range before constructing HttpListener.

The constructor accepts any integer for port and immediately uses it to build the HttpListener prefix. Invalid port values (outside 1-65535) will cause HttpListenerException when Start() is called, making diagnosis harder.

Apply this diff:
 public SimpleHttpServer(string nupkgPath, string releasesPath, int port, ILogger logger)
 {
+    if (port is < 1 or > 65535)
+        throw new ArgumentOutOfRangeException(nameof(port), "Port must be 1..65535.");
     _nupkgPath = nupkgPath ?? throw new ArgumentNullException(nameof(nupkgPath));
     _releasesPath = releasesPath ?? throw new ArgumentNullException(nameof(releasesPath));
     _logger = logger ?? throw new ArgumentNullException(nameof(logger));
     Port = port;
48-84: Reset state in Stop() to enable server restart.

Start() checks _serverTask != null at line 50 and returns early if true, but Stop() never resets _serverTask or _cts. After calling Stop(), calling Start() again becomes a no-op, and the cancellation token source is not disposed.

Apply this diff:
 public void Stop()
 {
     if (_cts == null || _serverTask == null)
     {
         return;
     }

     _cts.Cancel();
     _listener.Stop();

     try
     {
         _serverTask.Wait(TimeSpan.FromSeconds(5));
     }
     catch (Exception ex)
     {
         _logger.LogWarning(ex, "Error stopping HTTP server");
     }

     _logger.LogInformation("HTTP server stopped");
+
+    _serverTask = null;
+    _cts.Dispose();
+    _cts = null;
 }
114-172: Ensure response is always closed with a finally block.

If an exception occurs before line 166, the HttpListenerResponse may remain open, causing clients to hang and potentially exhausting listener resources.

Apply this diff:
 private async Task ProcessRequestAsync(HttpListenerContext context)
 {
+    var response = context.Response;
     try
     {
         var request = context.Request;
-        var response = context.Response;

         _logger.LogDebug("HTTP request: {Method} {Url}", request.HttpMethod, request.Url);

         // ... rest of request handling ...
-
-        response.Close();
     }
     catch (Exception ex)
     {
         _logger.LogError(ex, "Error processing HTTP request");
     }
+    finally
+    {
+        try { response.Close(); } catch { /* best-effort */ }
+    }
 }
94-112: Capture and log exceptions from fire-and-forget request tasks.

Line 101 discards the Task returned by Task.Run(...), so exceptions thrown in ProcessRequestAsync will be unobserved and may go unnoticed.

Apply this diff:
-                _ = Task.Run(() => ProcessRequestAsync(context), cancellationToken);
+                var requestTask = Task.Run(() => ProcessRequestAsync(context), cancellationToken);
+                _ = requestTask.ContinueWith(t =>
+                {
+                    if (t.IsFaulted && t.Exception != null)
+                        _logger.LogError(t.Exception, "Unhandled error in request processing");
+                }, TaskScheduler.Default);
GenHub/GenHub.Core/Models/AppUpdate/PullRequestInfo.cs (1)
53-56: Use null-safe comparison for IsOpen property.

The State.Equals(...) call will throw NullReferenceException if State is null. While State is marked required, it can still be set to null via reflection, deserialization, or unsafe code.

Apply this diff:
-    public bool IsOpen => State.Equals("open", StringComparison.OrdinalIgnoreCase);
+    public bool IsOpen => string.Equals(State, "open", StringComparison.OrdinalIgnoreCase);
GenHub/GenHub/Features/GitHub/Views/GitHubTokenDialogView.axaml.cs (1)
24-30: Fix event handler leak in SetViewModel.

Anonymous event handlers are attached to viewModel.SaveCompleted and viewModel.CancelRequested without removing previous handlers. If SetViewModel is called multiple times (e.g., dialog reused), handlers accumulate, preventing the previous ViewModel from being garbage collected.

Use named methods to enable proper cleanup:
+    private void OnSaveCompleted() => Close(true);
+    private void OnCancelRequested() => Close(false);
+
     public void SetViewModel(GitHubTokenDialogViewModel viewModel)
     {
+        if (DataContext is GitHubTokenDialogViewModel oldVm)
+        {
+            oldVm.SaveCompleted -= OnSaveCompleted;
+            oldVm.CancelRequested -= OnCancelRequested;
+        }
+
         DataContext = viewModel;

-        viewModel.SaveCompleted += () => Close(true);
-        viewModel.CancelRequested += () => Close(false);
+        viewModel.SaveCompleted += OnSaveCompleted;
+        viewModel.CancelRequested += OnCancelRequested;
     }
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/Services/VelopackUpdateManagerTests.cs (1)
164-169: Log assertion remains overly permissive (unresolved from previous review).

The assertion still uses OR logic that will match any log containing either "Velopack" OR "Update". A previous review requested tightening this to require both "Velopack" AND "UpdateManager" to match the actual warning message pattern.

Apply this diff:
                 It.Is<It.IsAnyType>((v, t) =>
-                    (v.ToString() ?? string.Empty).Contains("Velopack") ||
-                    (v.ToString() ?? string.Empty).Contains("Update")),
+                    (v.ToString() ?? string.Empty).Contains("Velopack") &&
+                    (v.ToString() ?? string.Empty).Contains("UpdateManager")),
GenHub/GenHub/Features/GitHub/ViewModels/GitHubTokenDialogViewModel.cs (2)
152-169: Scope validation may produce false positives with substring matching.

Line 155 uses Contains("repo", StringComparison.OrdinalIgnoreCase) which will match unintended scope names like "repository", "repos", or "repo:status".

Split the scope string and check for exact matches:
-                    if (scopeString.Contains("repo", StringComparison.OrdinalIgnoreCase))
+                    var scopes = scopeString.Split(',', ' ')
+                        .Select(s => s.Trim())
+                        .Where(s => !string.IsNullOrEmpty(s));
+                    if (scopes.Any(s => s.Equals("repo", StringComparison.OrdinalIgnoreCase)))
143-145: Do not dispose HttpClient created from IHttpClientFactory (unresolved from previous review).

Line 143 uses using var client = _httpClientFactory.CreateClient(); which disposes the HttpClient. This is incorrect for HttpClients created from IHttpClientFactory, as the factory manages their lifecycle and connection pooling. Disposing them can lead to socket exhaustion.

A previous review marked this as addressed in commits 2b43881 to 57ea8f9, but the code still shows the using var pattern.

Apply this diff:
-            using var client = _httpClientFactory.CreateClient();
+            var client = _httpClientFactory.CreateClient();
.github/workflows/ci.yml (3)
89-89: Update actions/cache to v4.

The workflow uses actions/cache@v3 which is outdated. Both actionlint and previous reviews recommend upgrading to v4 for consistency with other actions in this workflow.

Apply this diff:
       - name: Cache NuGet Packages
-        uses: actions/cache@v3
+        uses: actions/cache@v4
Also applies to line 247.

386-387: Release tag is hardcoded instead of using computed build version.

The release tag uses a hardcoded version format v0.0.${{ github.run_number }} which doesn't match the actual version computed in the Extract Build Info steps (lines 96-124 for Windows, 254-281 for Linux). This creates duplicate version logic that can diverge.

Consider exposing VERSION as a job output from the build-windows job and referencing it in the release step. Add after line 77:
    outputs:
      version: ${{ steps.buildinfo.outputs.VERSION }}
Then update lines 386-387:
-          tag_name: v0.0.${{ github.run_number }}
-          name: GenHub Alpha ${{ github.run_number }}
+          tag_name: v${{ needs.build-windows.outputs.version }}
+          name: GenHub Alpha ${{ needs.build-windows.outputs.version }}
254-281: CHANNEL is computed but never exported to GitHub Output on Linux (unresolved critical issue).

The Linux Extract Build Info step computes CHANNEL at line 268 but fails to export it to $GITHUB_OUTPUT. Only SHORT_HASH, PR_NUMBER, and VERSION are exported (lines 278-281).

Downstream build and publish steps reference ${{ steps.buildinfo.outputs.CHANNEL }} (lines 284, 290, 293), which will resolve to an empty string, causing assembly metadata to use the default value "Dev" instead of the intended "CI" or "PR" channel.

Add the missing export after line 281:
           echo "SHORT_HASH=$SHORT_HASH" >> $GITHUB_OUTPUT
           echo "PR_NUMBER=$PR_NUMBER" >> $GITHUB_OUTPUT
           echo "VERSION=$VERSION" >> $GITHUB_OUTPUT
+          echo "CHANNEL=$CHANNEL" >> $GITHUB_OUTPUT
GenHub/GenHub/Features/Settings/Views/SettingsView.axaml (2)
494-500: Ellipse.Fill binding expects a Brush, not a string color code.

The Fill property on Line 497 is bound to PatStatusColor, which likely returns a string (e.g., "#4CAF50"). However, Ellipse.Fill expects a Brush type. This binding will fail at runtime.

Either:

Change the ViewModel property to return a SolidColorBrush:
public SolidColorBrush PatStatusColor => 
    HasGitHubPat ? new SolidColorBrush(Color.Parse("#4CAF50")) 
                 : new SolidColorBrush(Color.Parse("#888888"));
Or create a value converter to convert hex strings to brushes and apply it to the binding:
<Ellipse Fill="{Binding PatStatusColor, Converter={StaticResource HexToBrushConverter}}" />
570-572: Invalid binding syntax: cannot negate an integer Count property.

Line 571 uses IsVisible="{Binding !AvailableArtifacts.Count}" which attempts to negate an integer. This is invalid in Avalonia bindings and will produce binding errors.

Add a boolean property to the ViewModel:
public bool HasNoArtifacts => AvailableArtifacts?.Count == 0;
Then update the binding:
-           IsVisible="{Binding !AvailableArtifacts.Count}"
+           IsVisible="{Binding HasNoArtifacts}"
Remember to raise PropertyChanged for HasNoArtifacts when AvailableArtifacts changes.
GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationView.axaml (1)
246-246: ObjectConverters requires xmlns prefix declaration.

Line 246 references Converter={x:Static ObjectConverters.IsNotNull} without declaring the namespace. This will fail XAML compilation since ObjectConverters isn't in the default Avalonia namespace.

Add the namespace declaration to the root <UserControl> element:
 <UserControl xmlns="https://github.com/avaloniaui"
     xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
     xmlns:vm="using:GenHub.Features.AppUpdate.ViewModels"
+    xmlns:conv="using:Avalonia.Data.Converters"
     x:Class="GenHub.Features.AppUpdate.Views.UpdateNotificationView"
Then update the converter reference:
-    IsVisible="{Binding SubscribedPr, Converter={x:Static ObjectConverters.IsNotNull}}">
+    IsVisible="{Binding SubscribedPr, Converter={x:Static conv:ObjectConverters.IsNotNull}}">
GenHub/GenHub/Features/Settings/ViewModels/SettingsViewModel.cs (5)
153-165: Fire-and-forget async call may swallow exceptions.

Line 165 uses _ = LoadPatStatusAsync() which discards the task. While LoadPatStatusAsync has internal try-catch, any unexpected exceptions will be unobserved.

Consider adding a continuation to log any unobserved failures:
-        _ = LoadPatStatusAsync();
+        _ = LoadPatStatusAsync().ContinueWith(t =>
+        {
+            if (t.IsFaulted && t.Exception != null)
+            {
+                _logger.LogError(t.Exception, "Failed to load PAT status");
+            }
+        }, TaskScheduler.Default);
709-724: LoadPatStatusAsync treats "token exists" as "token valid".

When HasGitHubPat is true (line 710), IsPatValid is set to true (line 713) without actually validating the token against the GitHub API. This will show a green state for expired/revoked tokens.

Consider splitting "configured" vs "validated" states, or performing an explicit validation call when the token exists.

768-785: PAT "validation" always succeeds after saving (logic bug).

After await _gitHubTokenStorage.SaveTokenAsync(secureString) on line 768, the condition artifact != null || _gitHubTokenStorage.HasToken() on line 778 will always be true because HasToken() returns true after saving. This means invalid PATs (unauthorized, insufficient scopes) will show "PAT validated successfully ✓".

Remove the || _gitHubTokenStorage.HasToken() clause:
-                    if (artifact != null || _gitHubTokenStorage.HasToken())
+                    if (artifact != null)
                     {
                         PatStatusMessage = "PAT validated successfully ✓";
Also handle the case where artifact == null but no exception was thrown (PAT works but no artifacts exist).

875-884: Marshal AvailableArtifacts mutations to the UI thread.

AvailableArtifacts.Clear() (line 876) and AvailableArtifacts.Add(artifact) (line 883) may execute off the UI thread after awaits, which can crash Avalonia-bound collections.
+using Avalonia.Threading;
         IsLoadingArtifacts = true;
-        AvailableArtifacts.Clear();
+        await Dispatcher.UIThread.InvokeAsync(() => AvailableArtifacts.Clear());
...
             if (artifact != null)
             {
-                AvailableArtifacts.Add(artifact);
+                await Dispatcher.UIThread.InvokeAsync(() => AvailableArtifacts.Add(artifact));
915-922: Add SaveAsync() call after updating the channel setting.

The Update() method on line 916 only persists changes in-memory. Per the interface documentation, SaveAsync() must be called to write to disk. The update channel change will be lost on restart.
         // Persist setting
         _userSettingsService.Update(s => s.UpdateChannel = value);
+        _ = _userSettingsService.SaveAsync();
 
         // If switching to Artifacts, load available artifacts
Also, line 921 uses fire-and-forget _ = LoadArtifactsAsync() without exception handling.
GenHub/GenHub/Features/AppUpdate/ViewModels/UpdateNotificationViewModel.cs (5)
142-143: Fire-and-forget async call may swallow exceptions.

Line 143 uses _ = InitializeAsync() which discards the task. Exceptions thrown will be unobserved.
-        _ = InitializeAsync();
+        _ = InitializeAsync().ContinueWith(t =>
+        {
+            if (t.IsFaulted && t.Exception != null)
+            {
+                _logger.LogError(t.Exception, "Initialization failed");
+            }
+        }, TaskScheduler.Default);
149-166: InitializeAsync lacks top-level exception handling.

The method calls LoadPullRequestsAsync and CheckForUpdatesAsync without a surrounding try-catch. Combined with the fire-and-forget pattern in the constructor, any exceptions will be unobserved.

Wrap the method body in try-catch:
 private async Task InitializeAsync()
 {
+    try
+    {
         var settings = _userSettingsService.Get();
         // ... existing code ...
         await CheckForUpdatesAsync();
+    }
+    catch (Exception ex)
+    {
+        _logger.LogError(ex, "Initialization failed");
+        HasError = true;
+        ErrorMessage = $"Initialization failed: {ex.Message}";
+    }
 }
608-609: AvailablePullRequests.Clear() is outside Dispatcher block.

Line 609 clears the collection before the Dispatcher.UIThread.InvokeAsync block on line 617. After await statements, code may resume on a non-UI thread, causing potential crashes.
         IsLoadingPullRequests = true;
-        AvailablePullRequests.Clear();
+        await Dispatcher.UIThread.InvokeAsync(() => AvailablePullRequests.Clear());
663-665: Fire-and-forget SaveAsync() without error handling.

Line 665 discards the save task. If saving fails, the error is silently lost and the user won't know their subscription wasn't persisted.
         _userSettingsService.Update(s => s.SubscribedPrNumber = prNumber);
-        _ = _userSettingsService.SaveAsync();
+        _ = _userSettingsService.SaveAsync().ContinueWith(t =>
+        {
+            if (t.IsFaulted)
+                _logger.LogError(t.Exception, "Failed to save PR subscription");
+        }, TaskScheduler.Default);
691-693: Fire-and-forget SaveAsync in UnsubscribeFromPr.

Same pattern as SubscribeToPr - the save operation's errors are discarded.

Consider extracting a helper method:
private void SaveSettingsFireAndForget()
{
    _ = _userSettingsService.SaveAsync().ContinueWith(t =>
    {
        if (t.IsFaulted)
            _logger.LogError(t.Exception, "Failed to save settings");
    }, TaskScheduler.Default);
}
GenHub/GenHub/Features/AppUpdate/Services/VelopackUpdateManager.cs (8)
62-79: Remove debug logging from hot property getters.

Lines 66 and 76 log debug messages on every property access. These properties are likely polled frequently by UI bindings, which will flood logs and add overhead.
 public bool HasUpdateAvailableFromGitHub
 {
     get
     {
-        logger.LogDebug("HasUpdateAvailableFromGitHub property accessed: {Value}", _hasUpdateFromGitHub);
         return _hasUpdateFromGitHub;
     }
 }

 public string? LatestVersionFromGitHub
 {
     get
     {
-        logger.LogDebug("LatestVersionFromGitHub property accessed: '{Value}'", _latestVersionFromGitHub ?? "NULL");
         return _latestVersionFromGitHub;
     }
 }
203-210: SecureString token not disposed after use.

LoadTokenAsync returns a SecureString which implements IDisposable. The token should be disposed after use to clear sensitive data from memory promptly.
-            var token = await gitHubTokenStorage.LoadTokenAsync();
+            using var token = await gitHubTokenStorage.LoadTokenAsync();
             if (token == null)
             {
                 logger.LogWarning("Failed to load GitHub PAT");
                 return null;
             }
319-324: SecureString token not disposed after use.

Same issue as in CheckForArtifactUpdatesAsync - wrap the token in a using statement.
-            var token = await gitHubTokenStorage.LoadTokenAsync();
+            using var token = await gitHubTokenStorage.LoadTokenAsync();
373-468: N+1 GitHub API pattern still present - fetches workflow runs inside PR loop.

For each open PR (lines 351-482), the code fetches workflow runs (lines 376-378) and then artifacts (line 413). With 30 PRs, this makes 30+ API calls, quickly exhausting rate limits.

Fetch workflow runs once before the PR loop, group by PR number, then look up runs for each PR:
// Fetch all recent successful runs once BEFORE the PR loop
var runsUrl = $"https://api.github.com/repos/{owner}/{repo}/actions/runs?status=success&per_page=100";
var runsResponse = await client.GetAsync(runsUrl, cancellationToken);
// ... parse and group by PR number ...

// Then inside the PR loop, just look up from the pre-fetched dictionary
foreach (var pr in prsData.EnumerateArray())
{
    // ... look up runs for this PR from grouped data ...
}
643-649: SecureString token not disposed after use.

Same pattern - wrap in using:
-            var token = await gitHubTokenStorage.LoadTokenAsync();
+            using var token = await gitHubTokenStorage.LoadTokenAsync();
786-806: Potential race condition with server disposal after ApplyUpdatesAndExit.

After calling ApplyUpdatesAndExit (line 791), the code waits 5 seconds then throws if the app didn't exit. However, the finally block (line 817) will dispose the HTTP server, potentially while Velopack is still downloading from it.

Consider not throwing an error if the exit doesn't happen immediately - the update may be staged for next launch:
-                logger.LogError("Application did not exit after ApplyUpdatesAndExit. Update may have failed.");
-                throw new InvalidOperationException("Application did not exit after applying update");
+                // ApplyUpdatesAndExit returned - update may be staged for next launch
+                logger.LogWarning("ApplyUpdatesAndExit returned - update staged for next launch");
+                progress?.Report(new UpdateProgress { Status = "Update staged - restart to apply", PercentComplete = 100 });
218-222: Differentiate 401/403 auth failures from empty results.

Both CheckForArtifactUpdatesAsync and GetOpenPullRequestsAsync return null/empty list for any HTTP error, including 401/403 unauthorized responses. This makes auth failures indistinguishable from legitimate empty states, breaking PAT validation in SettingsViewModel.

Consider checking for 401/403 status codes explicitly and either throwing an exception or returning a richer result type:
             if (!runsResponse.IsSuccessStatusCode)
             {
+                if (runsResponse.StatusCode == System.Net.HttpStatusCode.Unauthorized ||
+                    runsResponse.StatusCode == System.Net.HttpStatusCode.Forbidden)
+                {
+                    throw new UnauthorizedAccessException($"GitHub API authentication failed: {runsResponse.StatusCode}");
+                }
                 logger.LogWarning("Failed to fetch workflow runs: {Status}", runsResponse.StatusCode);
                 return null;
             }
868-875: TOCTOU race in FindAvailablePort.

The port is released immediately after listener.Stop(), creating a window where another process could claim it before the HTTP server binds.

This is a minor edge case. Consider either keeping the listener and passing it to the HTTP server, or adding retry logic if binding fails.

🧹 Nitpick comments (6)

GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/GameProfileModuleTests.cs (1)
260-260: Consider adding a using directive to simplify the fully qualified type name.

Line 260 uses the fully qualified name GenHub.Core.Interfaces.Shortcuts.IShortcutService twice, which reduces readability. Adding a using directive at the top of the file would make this line more concise and consistent with the pattern used for other interface registrations.

Add the using directive at the top of the file:
 using GenHub.Core.Interfaces.Manifest;
+using GenHub.Core.Interfaces.Shortcuts;
 using GenHub.Core.Interfaces.Storage;
Then simplify line 260:
-        services.AddSingleton<GenHub.Core.Interfaces.Shortcuts.IShortcutService>(new Mock<GenHub.Core.Interfaces.Shortcuts.IShortcutService>().Object);
+        services.AddSingleton<IShortcutService>(new Mock<IShortcutService>().Object);
GenHub/GenHub.Core/Constants/AppConstants.cs (1)
99-99: Consider using a static property for GitHubRepositoryUrl.

Line 99 uses const string concatenation, which works but is unconventional. Consider converting to a static property for clarity:
-    public const string GitHubRepositoryUrl = "https://github.com/" + GitHubRepositoryOwner + "/" + GitHubRepositoryName;
+    public static string GitHubRepositoryUrl => $"https://github.com/{GitHubRepositoryOwner}/{GitHubRepositoryName}";
This makes the computed nature explicit and uses string interpolation for readability.
GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationView.axaml (1)

7-340: Consider the layout complexity with multiple conditional sections.

The view has grown significantly with PR subscription UI, status bars, and artifact management. While functional, consider:

Extracting the PR list (lines 204-243) into a separate UserControl for reusability

Using a ViewModel property to ensure only one main content section is visible at a time instead of relying on multiple IsVisible bindings

These refactorings would improve maintainability as the feature evolves.
GenHub/GenHub/Features/AppUpdate/ViewModels/UpdateNotificationViewModel.cs (2)
542-546: Fire-and-forget SaveAsync in DismissUpdate.

Line 546 discards the save task for the dismissed version. Apply the same error-handling pattern as suggested for other save operations.
-            _ = _userSettingsService.SaveAsync();
+            _ = _userSettingsService.SaveAsync().ContinueWith(t =>
+            {
+                if (t.IsFaulted)
+                    _logger.LogError(t.Exception, "Failed to save dismissed version");
+            }, TaskScheduler.Default);
657-672: Handle case where PR number is not found in available PRs.

If prNumber is not in AvailablePullRequests, SubscribedPr becomes null (line 660) while _velopackUpdateManager.SubscribedPrNumber is set (line 659). This creates inconsistent state where the manager thinks we're subscribed but the ViewModel has no PR info.
     private void SubscribeToPr(int prNumber)
     {
+        var pr = AvailablePullRequests.FirstOrDefault(p => p.Number == prNumber);
+        if (pr == null)
+        {
+            _logger.LogWarning("Attempted to subscribe to PR #{PrNumber} which is not in available PRs", prNumber);
+            return;
+        }
+
         _velopackUpdateManager.SubscribedPrNumber = prNumber;
-        SubscribedPr = AvailablePullRequests.FirstOrDefault(p => p.Number == prNumber);
+        SubscribedPr = pr;
GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/SharedViewModelModuleTests.cs (1)
115-121: Consider adding using statements for consistency.

The fully qualified names GenHub.Core.Interfaces.Shortcuts.IShortcutService and GenHub.Core.Interfaces.GitHub.IGitHubTokenStorage are inconsistent with the rest of the file's style, which uses using statements for interfaces.

Add these using statements at the top of the file:
+using GenHub.Core.Interfaces.GitHub;
+using GenHub.Core.Interfaces.Shortcuts;
Then simplify the registrations:
-        // Mock IShortcutService (platform-specific service)
-        var shortcutServiceMock = new Mock<GenHub.Core.Interfaces.Shortcuts.IShortcutService>();
-        services.AddSingleton<GenHub.Core.Interfaces.Shortcuts.IShortcutService>(shortcutServiceMock.Object);
+        // Mock IShortcutService (platform-specific service)
+        var shortcutServiceMock = new Mock<IShortcutService>();
+        services.AddSingleton<IShortcutService>(shortcutServiceMock.Object);

-        // Mock IGitHubTokenStorage (platform-specific service)
-        var gitHubTokenStorageMock = new Mock<GenHub.Core.Interfaces.GitHub.IGitHubTokenStorage>();
-        services.AddSingleton<GenHub.Core.Interfaces.GitHub.IGitHubTokenStorage>(gitHubTokenStorageMock.Object);
+        // Mock IGitHubTokenStorage (platform-specific service)
+        var gitHubTokenStorageMock = new Mock<IGitHubTokenStorage>();
+        services.AddSingleton<IGitHubTokenStorage>(gitHubTokenStorageMock.Object);

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ede51ab and 3283582.

📒 Files selected for processing (30)

.github/workflows/ci.yml (5 hunks)
GenHub/Directory.Build.props (1 hunks)
GenHub/GenHub.Core/Constants/AppConstants.cs (5 hunks)
GenHub/GenHub.Core/Constants/BuildInfoConstants.cs (1 hunks)
GenHub/GenHub.Core/Constants/GitHubConstants.cs (1 hunks)
GenHub/GenHub.Core/Helpers/FileSizeFormatter.cs (1 hunks)
GenHub/GenHub.Core/Models/AppUpdate/ArtifactUpdateInfo.cs (1 hunks)
GenHub/GenHub.Core/Models/AppUpdate/PullRequestInfo.cs (1 hunks)
GenHub/GenHub.Core/Models/Common/UserSettings.cs (2 hunks)
GenHub/GenHub.Core/Models/Enums/UpdateChannel.cs (1 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Constants/AppConstantsTests.cs (1 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/Services/VelopackUpdateManagerTests.cs (12 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/ViewModels/UpdateNotificationViewModelTests.cs (4 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameProfiles/GameProcessManagerTests.cs (1 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/GameProfileModuleTests.cs (5 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/SharedViewModelModuleTests.cs (3 hunks)
GenHub/GenHub/Common/ViewModels/MainViewModel.cs (1 hunks)
GenHub/GenHub/Features/AppUpdate/Interfaces/IVelopackUpdateManager.cs (3 hunks)
GenHub/GenHub/Features/AppUpdate/Services/SimpleHttpServer.cs (1 hunks)
GenHub/GenHub/Features/AppUpdate/Services/VelopackUpdateManager.cs (7 hunks)
GenHub/GenHub/Features/AppUpdate/ViewModels/UpdateNotificationViewModel.cs (9 hunks)
GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationView.axaml (5 hunks)
GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationWindow.axaml (4 hunks)
GenHub/GenHub/Features/GameProfiles/Services/ProfileContentLoader.cs (5 hunks)
GenHub/GenHub/Features/GitHub/ViewModels/GitHubTokenDialogViewModel.cs (1 hunks)
GenHub/GenHub/Features/GitHub/Views/GitHubTokenDialogView.axaml (1 hunks)
GenHub/GenHub/Features/GitHub/Views/GitHubTokenDialogView.axaml.cs (1 hunks)
GenHub/GenHub/Features/Settings/ViewModels/SettingsViewModel.cs (8 hunks)
GenHub/GenHub/Features/Settings/Views/SettingsView.axaml (18 hunks)
GenHub/GenHub/Features/Settings/Views/SettingsView.axaml.cs (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (6)

GenHub/GenHub.Core/Models/Common/UserSettings.cs
GenHub/GenHub.Core/Models/Enums/UpdateChannel.cs
GenHub/GenHub/Features/Settings/Views/SettingsView.axaml.cs
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameProfiles/GameProcessManagerTests.cs
GenHub/GenHub.Core/Helpers/FileSizeFormatter.cs
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/ViewModels/UpdateNotificationViewModelTests.cs

🧰 Additional context used

🧬 Code graph analysis (8)

GenHub/GenHub.Tests/GenHub.Tests.Core/Constants/AppConstantsTests.cs (1)

GenHub/GenHub.Core/Constants/AppConstants.cs (1)

AppConstants (10-137)

GenHub/GenHub/Features/GitHub/Views/GitHubTokenDialogView.axaml.cs (2)

GenHub/GenHub/Features/Settings/Views/SettingsView.axaml.cs (1)

InitializeComponent (76-79)

GenHub/GenHub/Features/GitHub/ViewModels/GitHubTokenDialogViewModel.cs (4)

GitHubTokenDialogViewModel (19-265)

GitHubTokenDialogViewModel (54-56)

GitHubTokenDialogViewModel (64-72)

SetToken (78-93)

GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/Services/VelopackUpdateManagerTests.cs (2)

GenHub/GenHub/Features/AppUpdate/Services/VelopackUpdateManager.cs (7)

HttpClient (918-934)

Task (93-184)

Task (187-298)

Task (301-513)

Task (516-560)

Task (624-831)

VelopackUpdateManager (30-935)

GenHub/GenHub/Features/AppUpdate/Interfaces/IVelopackUpdateManager.cs (5)

Task (21-21)

Task (29-29)

Task (37-37)

Task (46-46)

Task (109-109)

GenHub/GenHub/Features/GitHub/ViewModels/GitHubTokenDialogViewModel.cs (2)

GenHub/GenHub.Core/Constants/GitHubConstants.cs (1)

GitHubConstants (4-335)

GenHub/GenHub.Core/Constants/AppConstants.cs (1)

AppConstants (10-137)

GenHub/GenHub/Features/AppUpdate/Services/SimpleHttpServer.cs (1)

GenHub/GenHub/Features/AppUpdate/Services/VelopackUpdateManager.cs (1)

Dispose (82-90)

GenHub/GenHub/Features/AppUpdate/Services/VelopackUpdateManager.cs (5)

GenHub/GenHub.Core/Constants/AppConstants.cs (1)

AppConstants (10-137)

GenHub/GenHub/Infrastructure/Extensions/LoggerExtensions.cs (1)

LogError (46-51)

GenHub/GenHub/Features/AppUpdate/Interfaces/IVelopackUpdateManager.cs (5)

Task (21-21)

Task (29-29)

Task (37-37)

Task (46-46)

Task (109-109)

GenHub/GenHub/Features/AppUpdate/Services/SimpleHttpServer.cs (4)

Task (94-112)

Task (114-172)

SimpleHttpServer (13-173)

SimpleHttpServer (34-43)

GenHub/GenHub/Features/AppUpdate/ViewModels/UpdateNotificationViewModel.cs (2)

Task (149-167)

Task (243-367)

GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/GameProfileModuleTests.cs (3)

GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/SharedViewModelModuleTests.cs (1)

IConfigurationProviderService (142-159)

GenHub/GenHub/Features/GameProfiles/ViewModels/GameSettingsViewModel.cs (1)

Core (183-203)

GenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameProfiles/ViewModels/MainViewModelTests.cs (1)

Mock (328-335)

GenHub/GenHub/Features/GameProfiles/Services/ProfileContentLoader.cs (3)

GenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileSettingsViewModel.cs (2)

ObservableCollection (1279-1281)

ContentDisplayItem (1257-1272)

GenHub/GenHub.Core/Models/Content/ContentDisplayItem.cs (1)

ContentDisplayItem (11-234)

GenHub/GenHub/Features/GameProfiles/Services/ContentDisplayFormatter.cs (2)

ContentDisplayItem (26-46)

ContentDisplayItem (49-73)

🪛 actionlint (1.7.9)

.github/workflows/ci.yml

89-89: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

247-247: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

🪛 GitHub Check: Build Linux

GenHub/GenHub.Core/Models/AppUpdate/PullRequestInfo.cs

[warning] 31-31:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 26-26:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 21-21:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 16-16:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 11-11:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 31-31:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 26-26:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 21-21:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 16-16:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 11-11:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: Build Windows

🔇 Additional comments (16)

GenHub/GenHub/Common/ViewModels/MainViewModel.cs (1)

428-428: Trivial formatting change - no review needed.

This is purely an end-of-file newline adjustment with no functional impact.

GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/GameProfileModuleTests.cs (1)

305-305: Good addition of trailing newline.

Adding the trailing newline at the end of the file follows best practices and prevents potential issues with text processing tools.

GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationWindow.axaml (2)

32-33: LGTM: Background gradient updated for a more neutral aesthetic.

The gradient color change from bluish-gray to neutral dark gray is a purely visual design update. The text contrast remains appropriate with the light-colored foreground elements.

48-58: LGTM: Simplified icon rendering.

Replacing the custom Path-based icon with an Image element simplifies the markup and makes the icon easier to maintain. The icon is now consistently referenced as an asset throughout the window.

GenHub/GenHub/Features/GameProfiles/Services/ProfileContentLoader.cs (1)

495-495: Good improvement to readability with named tuple elements.

Adding explicit names (Installation and Client) to the tuple return type improves code clarity and makes the method signature self-documenting. The change is backward-compatible with existing deconstruction patterns (line 470).

GenHub/GenHub.Tests/GenHub.Tests.Core/Constants/AppConstantsTests.cs (1)

24-31: LGTM!

The test expectations correctly reflect the new 0.0.x alpha versioning scheme and DisplayVersion format.

GenHub/GenHub.Core/Constants/GitHubConstants.cs (1)

280-281: LGTM!

The PatCreationUrl constant correctly points to GitHub's PAT creation page with appropriate scope and description pre-filled.

GenHub/GenHub/Features/GitHub/Views/GitHubTokenDialogView.axaml (1)

1-170: LGTM!

The dialog view correctly implements the GitHub token entry UI with proper MVVM bindings. The Save button is appropriately gated by IsTokenValid, and the password masking provides reasonable security given the SecureString backend in the ViewModel.

GenHub/GenHub/Features/AppUpdate/Interfaces/IVelopackUpdateManager.cs (1)

23-110: LGTM! Clean interface expansion for artifact and PR update features.

The new methods and properties are well-documented and follow consistent patterns with the existing interface. The additions for artifact updates, PR management, and channel selection align well with the broader PR objectives.

GenHub/GenHub.Core/Constants/AppConstants.cs (1)

27-94: Clean implementation of runtime build metadata extraction.

The use of Lazy<T> for metadata properties and the GetAssemblyMetadata helper provide a clean, performant way to expose CI/CD build information. The FullDisplayVersion property nicely handles the different display formats for local, CI, and PR builds.

GenHub/Directory.Build.props (1)

1-44: Well-structured build metadata integration.

The versioning scheme change to 0.0.x alpha and the CI metadata injection pattern are clean and maintainable. The conditional InformationalVersion ensures git hash is included in CI builds while remaining clean for local development. The AssemblyMetadataAttribute entries enable runtime access to build information.

GenHub/GenHub/Features/AppUpdate/Services/VelopackUpdateManager.cs (2)

30-37: Constructor initializes fields that capture constructor parameters.

Lines 36-37 initialize _githubSource and _updateManager as field initializers, which execute before the constructor body. The logger parameter is used in CreateUpdateManager(logger) on line 37, which works with primary constructors but can be confusing.

The primary constructor pattern is used correctly here. The DI-based initialization looks good.

92-184: LGTM! Update check flow with stale state reset.

The method properly resets _hasUpdateFromGitHub and _latestVersionFromGitHub at the start (lines 98-99), addressing the stale state issue from past reviews. Channel filtering (lines 140-141) and fallback to Velopack UpdateManager (lines 160-175) are implemented correctly.

GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/SharedViewModelModuleTests.cs (3)

104-104: LGTM - CasServices registration reordered.

The reordering of AddCasServices() earlier in the sequence appears correct based on the AI summary indicating a duplicate registration was removed.

27-140: Test structure is sound.

The test properly verifies that the DI container can resolve the registered ViewModels. The fix for the duplicate IGitHubTokenStorage registration (from the previous review) has been correctly applied, and the registration order changes appear intentional.

110-121: No action required. The IHttpClientFactory dependency for VelopackUpdateManager is properly registered by AddAppUpdateModule(), which is already called in the test setup on line 110.

Likely an incorrect or invalid review comment.

GenHub/GenHub.Core/Models/AppUpdate/PullRequestInfo.cs

coderabbitai · 2025-12-16T20:46:42Z

GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationWindow.axaml

        Width="580" Height="800"
        MinWidth="500" MinHeight="580"
        Title="GenHub Updates"
+        Icon="/Assets/Icons/generalshub-icon.png"


⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Verify the icon file exists fd -t f "generalshub-icon.png" GenHub/GenHub/Assets/Icons/ 2>/dev/null || echo "File not found"

Repository: undead2146/GenHub

Length of output: 108

🏁 Script executed:

# Check the XAML file structure around the lines in question head -60 GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationWindow.axaml | tail -55

Repository: undead2146/GenHub

Length of output: 2293

Mark the decorative icon as non-interactive for accessibility.

The icon file exists and is correctly referenced. However, the Image element on lines 50-52 should be marked as decorative to prevent screen readers from treating it separately from the accompanying "GenHub Updates" text label.

Apply this change:

<Image Source="/Assets/Icons/generalshub-icon.png" Width="28" Height="28" - Margin="8,0,0,0" /> + Margin="8,0,0,0" + AutomationProperties.AccessibilityView="Raw" />

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents

In GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationWindow.axaml around lines 50 to 52, the Image used for the icon should be marked decorative so screen readers ignore it; update the Image element to set AutomationProperties.IsInAccessibleTree="False" and IsHitTestVisible="False" (or equivalent) so it is non-interactive and excluded from the accessibility tree while leaving the visible icon intact next to the "GenHub Updates" label.

coderabbitai

Actionable comments posted: 2

♻️ Duplicate comments (27)

GenHub/GenHub/Features/GameProfiles/Services/ProfileContentLoader.cs (2)
400-405: Avoid double enumeration of the LINQ query.

The items.Count() call on Line 404 enumerates the IEnumerable, and then new ObservableCollection<ContentDisplayItem>(items) on Line 405 enumerates it again. Materialize the collection first to avoid unnecessary re-enumeration.

Apply this diff to materialize the collection:
     var items = manifestsResult.Data
         .Where(m => m.ContentType == contentType)
-        .Select(m => CreateManifestDisplayItem(m, isEnabled: enabledIds.Contains(m.Id.Value)));
+        .Select(m => CreateManifestDisplayItem(m, isEnabled: enabledIds.Contains(m.Id.Value)))
+        .ToList();

     logger.LogInformation("Loaded {Count} content items for type {ContentType}", items.Count, contentType);
     return new ObservableCollection<ContentDisplayItem>(items);
231-251: Incomplete property cloning causes data loss.

The CloneWithEnabledState method only copies a subset of ContentDisplayItem properties. Properties like ReleaseDate, FileSize, IconUrl, IsInstalled, IsAvailable, Tags, Metadata, Dependencies, and others are not copied, resulting in data loss when items are cloned.

Consider implementing a proper clone method on ContentDisplayItem or copying all relevant properties:
 private static ObservableCollection<ContentDisplayItem> CloneWithEnabledState(
     ObservableCollection<ContentDisplayItem> items,
     HashSet<string> enabledIds)
 {
     return new ObservableCollection<ContentDisplayItem>(
         items.Select(item => new ContentDisplayItem
         {
             Id = item.Id,
             ManifestId = item.ManifestId,
             DisplayName = item.DisplayName,
             Description = item.Description,
             Version = item.Version,
             ContentType = item.ContentType,
             GameType = item.GameType,
             InstallationType = item.InstallationType,
             Publisher = item.Publisher,
             SourceId = item.SourceId,
             GameClientId = item.GameClientId,
             IsEnabled = enabledIds.Contains(item.ManifestId),
+            ReleaseDate = item.ReleaseDate,
+            FileSize = item.FileSize,
+            IconUrl = item.IconUrl,
+            DownloadUrl = item.DownloadUrl,
+            IsInstalled = item.IsInstalled,
+            IsAvailable = item.IsAvailable,
+            Tags = new List<string>(item.Tags),
+            Metadata = new Dictionary<string, string>(item.Metadata),
+            Dependencies = new List<string>(item.Dependencies),
+            IsRequired = item.IsRequired,
+            StatusMessage = item.StatusMessage,
+            Category = item.Category,
+            SourceTypeName = item.SourceTypeName,
+            InstallPath = item.InstallPath,
+            BuildDate = item.BuildDate,
+            Manifest = item.Manifest,
         }));
 }
GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationWindow.axaml (1)
50-52: Mark the decorative icon as non-interactive for accessibility.

The icon Image should be excluded from the accessibility tree since it's purely decorative and accompanies the "GenHub Updates" text label.

Apply this diff to add accessibility attributes:
 <Image Source="/Assets/Icons/generalshub-icon.png"
        Width="28" Height="28"
-       Margin="8,0,0,0" />
+       Margin="8,0,0,0"
+       AutomationProperties.AccessibilityView="Raw" />
.github/workflows/ci.yml (2)
254-281: Critical: CHANNEL is not exported to GITHUB_OUTPUT on Linux.

The Linux Extract Build Info step computes CHANNEL (lines 265, 268) but fails to export it to $GITHUB_OUTPUT. Lines 278-280 only export SHORT_HASH, PR_NUMBER, and VERSION. Downstream steps at lines 284 and 297 reference ${{ steps.buildinfo.outputs.CHANNEL }}, which will be empty, causing incorrect build metadata.

Add the missing export after line 280:
           echo "SHORT_HASH=$SHORT_HASH" >> $GITHUB_OUTPUT
           echo "PR_NUMBER=$PR_NUMBER" >> $GITHUB_OUTPUT
           echo "VERSION=$VERSION" >> $GITHUB_OUTPUT
+          echo "CHANNEL=$CHANNEL" >> $GITHUB_OUTPUT
351-390: Major: Release tag uses hardcoded version instead of computed build version.

Lines 386-387 hardcode the release tag as v0.0.${{ github.run_number }}, which diverges from the actual package versions computed in the build jobs. If the build logic changes or includes PR suffixes, the release tag won't reflect the actual artifacts.

To fix this, expose the computed version as a job output and reference it in the release step.

Add outputs to the build-windows job header (after line 77):
   build-windows:
     name: Build Windows
+    outputs:
+      version: ${{ steps.buildinfo.outputs.VERSION }}
     needs: detect-changes
Then update the release step to use the computed version:
-          tag_name: v0.0.${{ github.run_number }}
-          name: GenHub Alpha ${{ github.run_number }}
+          tag_name: v${{ needs.build-windows.outputs.version }}
+          name: GenHub Alpha ${{ needs.build-windows.outputs.version }}
GenHub/GenHub.Core/Models/AppUpdate/ArtifactUpdateInfo.cs (2)
22-22: Consider using DateTimeOffset for UTC timestamps.

GitHub timestamps are UTC. Using DateTimeOffset instead of DateTime avoids ambiguity about the Kind and ensures proper timezone handling.

Apply this diff:
-    DateTime createdAt)
+    DateTimeOffset createdAt)
 {
     // ...
-    public DateTime CreatedAt { get; init; } = createdAt;
+    public DateTimeOffset CreatedAt { get; init; } = createdAt;
Also applies to: 62-62

72-85: Guard DisplayVersion formatting for empty GitHash.

Consider omitting the ({GitHash}) suffix when GitHash is empty or whitespace to prevent odd UI strings like "v0.0.1 ()".

Apply this diff:
     public string DisplayVersion
     {
         get
         {
+            var hashPart = string.IsNullOrWhiteSpace(GitHash) ? string.Empty : $" ({GitHash})";
             if (PullRequestNumber.HasValue)
             {
                 // Strip any build metadata from version (everything after +)
                 var baseVersion = Version.Split('+')[0];
-                return $"v{baseVersion} ({GitHash})";
+                return $"v{baseVersion}{hashPart}";
             }

-            return $"v{Version} ({GitHash})";
+            return $"v{Version}{hashPart}";
         }
     }
GenHub/GenHub/Features/Settings/Views/SettingsView.axaml (2)
494-500: Verify that PatStatusColor returns a Brush type.

The Ellipse.Fill property expects a Brush, but if PatStatusColor returns a string (e.g., "#4CAF50"), this binding will fail at runtime. Ensure the ViewModel property returns a SolidColorBrush or use a value converter.

Run the following script to check the property type:
#!/bin/bash
# Description: Verify PatStatusColor property returns Brush type

# Search for PatStatusColor property implementation
rg -n "PatStatusColor" --type=cs -A 5 -B 2
570-572: Fix invalid IsVisible binding syntax.

The binding IsVisible="{Binding !AvailableArtifacts.Count}" attempts to negate an integer, which is invalid in Avalonia bindings. Add a boolean property to the ViewModel or use a converter.

Solution 1 (recommended): Add a boolean property to ViewModel
public bool HasNoArtifacts => AvailableArtifacts?.Count == 0;
Then bind:
-           IsVisible="{Binding !AvailableArtifacts.Count}"
+           IsVisible="{Binding HasNoArtifacts}"
Solution 2: Use a comparison converter
Create and register a converter that checks if Count equals 0.
GenHub/GenHub.Core/Models/AppUpdate/PullRequestInfo.cs (2)
11-11: Fix modifier order to comply with StyleCop SA1206.

The public modifier should appear before required for all properties to satisfy StyleCop SA1206.

Apply this diff:
-    required public int Number { get; init; }
+    public required int Number { get; init; }

-    required public string Title { get; init; }
+    public required string Title { get; init; }

-    required public string BranchName { get; init; }
+    public required string BranchName { get; init; }

-    required public string Author { get; init; }
+    public required string Author { get; init; }

-    required public string State { get; init; }
+    public required string State { get; init; }
Also applies to: 16-16, 21-21, 26-26, 31-31

56-56: Use null-safe string comparison for IsOpen property.

State is required but nullable at runtime. Calling State.Equals(...) will throw if State is null. Use a static comparison method instead.

Apply this diff:
-    public bool IsOpen => State.Equals("open", StringComparison.OrdinalIgnoreCase);
+    public bool IsOpen => string.Equals(State, "open", StringComparison.OrdinalIgnoreCase);
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/Services/VelopackUpdateManagerTests.cs (1)
164-169: Log assertion remains overly permissive.

The assertion uses OR logic: Contains("Velopack") || Contains("Update"), which will match many unrelated log messages. Consider using AND logic or matching a more specific pattern.

Apply this diff:
                 It.Is<It.IsAnyType>((v, t) =>
-                    (v.ToString() ?? string.Empty).Contains("Velopack") ||
-                    (v.ToString() ?? string.Empty).Contains("Update")),
+                    (v.ToString() ?? string.Empty).Contains("Velopack") &&
+                    (v.ToString() ?? string.Empty).Contains("UpdateManager")),
GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationView.axaml (1)
246-246: ObjectConverters.IsNotNull converter reference still lacks xmlns prefix.

This past review issue appears unresolved. Line 246 references ObjectConverters.IsNotNull without a namespace prefix, which will fail XAML compilation at runtime unless ObjectConverters is in the default Avalonia namespace (which it typically isn't).

Add the namespace mapping to the root element:
 <UserControl xmlns="https://github.com/avaloniaui"
     xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
     xmlns:vm="using:GenHub.Features.AppUpdate.ViewModels"
+    xmlns:conv="using:Avalonia.Data.Converters"
     x:Class="GenHub.Features.AppUpdate.Views.UpdateNotificationView"
     x:DataType="vm:UpdateNotificationViewModel">
Then update line 246:
-    <Border ... IsVisible="{Binding SubscribedPr, Converter={x:Static ObjectConverters.IsNotNull}}">
+    <Border ... IsVisible="{Binding SubscribedPr, Converter={x:Static conv:ObjectConverters.IsNotNull}}">
GenHub/GenHub/Features/Settings/ViewModels/SettingsViewModel.cs (3)
777-785: PAT validation still accepts invalid tokens (unresolved past issue).

The validation logic at line 778 still contains || _gitHubTokenStorage.HasToken(), which marks any saved token as valid even when the API call fails with 401/403. This means invalid/expired PATs will show "PAT validated successfully ✓" to users.

Remove the || _gitHubTokenStorage.HasToken() clause:
                     var artifact = await _updateManager.CheckForArtifactUpdatesAsync();
-                    if (artifact != null || _gitHubTokenStorage.HasToken())
+                    if (artifact != null)
                     {
                         PatStatusMessage = "PAT validated successfully ✓";
                         IsPatValid = true;
                         HasGitHubPat = true;
                         GitHubPatInput = string.Empty;
                         return;
                     }
915-917: Update channel change not persisted (unresolved past issue).

Line 916 uses Update() without calling SaveAsync(), so the channel change won't persist across app restarts. The past review comment on this issue remains unaddressed.

Add SaveAsync after the Update call:
         // Persist setting
         _userSettingsService.Update(s => s.UpdateChannel = value);
+        _ = _userSettingsService.SaveAsync().ContinueWith(t =>
+        {
+            if (t.IsFaulted)
+                _logger.LogError(t.Exception, "Failed to save update channel setting");
+        }, TaskScheduler.Default);
875-883: ObservableCollection mutations need UI thread marshaling (unresolved past issue).

Lines 876 and 883 mutate AvailableArtifacts without ensuring execution on the UI thread. After the await at line 880, execution may resume on a background thread, which can crash Avalonia-bound collections.

Marshal collection operations to the UI thread:
         IsLoadingArtifacts = true;
-        AvailableArtifacts.Clear();
+        await Dispatcher.UIThread.InvokeAsync(() => AvailableArtifacts.Clear());

         try
         {
             var artifact = await _updateManager.CheckForArtifactUpdatesAsync();
             if (artifact != null)
             {
-                AvailableArtifacts.Add(artifact);
+                await Dispatcher.UIThread.InvokeAsync(() => AvailableArtifacts.Add(artifact));
                 PatStatusMessage = $"Found {AvailableArtifacts.Count} artifact(s)";
             }
Don't forget to add using Avalonia.Threading; at the top of the file.
GenHub/GenHub/Features/AppUpdate/ViewModels/UpdateNotificationViewModel.cs (2)
549-549: Fire-and-forget SaveAsync calls still swallow errors (unresolved past issue).

Lines 549, 668, and 696 all use _ = _userSettingsService.SaveAsync() which discards the task. If saving fails, the error is silently lost and users won't know their settings weren't persisted.

Add error handling to all SaveAsync calls:
-        _ = _userSettingsService.SaveAsync();
+        _ = _userSettingsService.SaveAsync().ContinueWith(t =>
+        {
+            if (t.IsFaulted)
+                _logger.LogError(t.Exception, "Failed to save settings");
+        }, TaskScheduler.Default);
Also applies to: 668-668, 696-696

611-612: AvailablePullRequests.Clear() still not marshaled to UI thread (unresolved past issue).

Line 612 calls AvailablePullRequests.Clear() before the await at line 618, but it should be wrapped in Dispatcher.UIThread.InvokeAsync() since the method can be called from any thread.

Marshal the Clear operation:
         IsLoadingPullRequests = true;
-        AvailablePullRequests.Clear();
+        await Dispatcher.UIThread.InvokeAsync(() => AvailablePullRequests.Clear());
GenHub/GenHub/Features/AppUpdate/Services/VelopackUpdateManager.cs (3)
62-79: Debug logging in property getters still present (unresolved past issue).

Lines 66 and 76 still contain logger.LogDebug() calls in property getters that are likely polled frequently by UI bindings. This was flagged in past reviews but remains unaddressed despite being marked as resolved.

Remove the debug logging from getters:
 public bool HasUpdateAvailableFromGitHub
 {
     get
     {
-        logger.LogDebug("HasUpdateAvailableFromGitHub property accessed: {Value}", _hasUpdateFromGitHub);
         return _hasUpdateFromGitHub;
     }
 }

 public string? LatestVersionFromGitHub
 {
     get
     {
-        logger.LogDebug("LatestVersionFromGitHub property accessed: '{Value}'", _latestVersionFromGitHub ?? "NULL");
         return _latestVersionFromGitHub;
     }
 }
If access tracking is needed, log state changes in the setter methods instead.

203-210: SecureString tokens not disposed after use (unresolved past issue).

Lines 203, 319, and 643 load SecureString tokens without disposing them. Past reviews flagged this issue (marked as "Addressed in commit 669d18f"), but the code still doesn't use using statements for the tokens.

Wrap SecureString loads in using statements:
-            var token = await gitHubTokenStorage.LoadTokenAsync();
+            using var token = await gitHubTokenStorage.LoadTokenAsync();
             if (token == null)
             {
                 logger.LogWarning("Failed to load GitHub PAT");
                 return null;
             }
Apply this fix at all three locations: lines 203, 319, and 643.

Also applies to: 319-326, 643-647

373-468: N+1 GitHub API pattern still present in PR artifact loading (unresolved past issue).

Lines 376-377 fetch workflow runs inside the foreach (var pr in prsData.EnumerateArray()) loop, creating an N+1 API pattern. With 30 PRs, this makes 30+ API calls, quickly exhausting rate limits. Past reviews flagged this (marked as "Addressed in commits 06eddfc to e867ce6"), but the code still fetches runs per-PR.

Fetch workflow runs once before the PR loop:
// Fetch all runs once before the PR loop
var runsUrl = $"https://api.github.com/repos/{owner}/{repo}/actions/runs?status=success&per_page=100";
var runsResponse = await client.GetAsync(runsUrl, cancellationToken);

Dictionary<int, JsonElement> runsByPr = new();
if (runsResponse.IsSuccessStatusCode)
{
    var runsJson = await runsResponse.Content.ReadAsStringAsync(cancellationToken);
    var runsData = JsonSerializer.Deserialize<JsonElement>(runsJson);
    
    if (runsData.TryGetProperty("workflow_runs", out var runs))
    {
        // Group runs by PR number
        foreach (var run in runs.EnumerateArray())
        {
            if (run.TryGetProperty("pull_requests", out var runPrs) && runPrs.GetArrayLength() > 0)
            {
                var prNum = runPrs[0].GetProperty("number").GetInt32();
                if (!runsByPr.ContainsKey(prNum))
                {
                    runsByPr[prNum] = run; // Keep first (most recent) run per PR
                }
            }
        }
    }
}

foreach (var pr in prsData.EnumerateArray())
{
    var prNumber = pr.GetProperty("number").GetInt32();
    // ... existing PR property extraction ...
    
    // Find artifact using pre-fetched runs
    ArtifactUpdateInfo? latestArtifact = null;
    if (runsByPr.TryGetValue(prNumber, out var run))
    {
        // Fetch artifacts only for this run
        // ... rest of artifact logic ...
    }
}
GenHub/GenHub/Features/GitHub/ViewModels/GitHubTokenDialogViewModel.cs (5)
143-143: Remove disposal of HttpClient created from IHttpClientFactory.

Line 143 uses using var client = _httpClientFactory.CreateClient();, which disposes the client. HttpClients from IHttpClientFactory should NOT be disposed as the factory manages their lifecycle. This was flagged in a previous review but appears to still be present in the code.

Apply this diff:
-            using var client = _httpClientFactory.CreateClient();
+            var client = _httpClientFactory.CreateClient();
152-169: Scope check may produce false positives with substring matching.

Line 155 uses Contains("repo", StringComparison.OrdinalIgnoreCase), which could match unintended scope names like "repository" or "repos" instead of the exact "repo" scope.

Apply this diff for precise scope matching:
-                    if (scopeString.Contains("repo", StringComparison.OrdinalIgnoreCase))
+                    var scopeList = scopeString.Split(',').Select(s => s.Trim());
+                    if (scopeList.Any(s => s.Equals("repo", StringComparison.OrdinalIgnoreCase)))
78-93: Race condition: _secureToken is not thread-safe.

If SetToken is called while ValidateTokenAsync or SaveAsync is executing, _secureToken could be disposed mid-operation, causing ObjectDisposedException or data corruption.

Consider using a SemaphoreSlim to serialize access:
+    private readonly SemaphoreSlim _tokenLock = new SemaphoreSlim(1, 1);
+
     public void SetToken(string token)
     {
+        _tokenLock.Wait();
+        try
+        {
             _secureToken?.Dispose();
             _secureToken = new SecureString();
             foreach (var c in token)
             {
                 _secureToken.AppendChar(c);
             }
 
             _secureToken.MakeReadOnly();
 
             // Clear validation when token changes
             ValidationMessage = string.Empty;
             HasValidationMessage = false;
             IsTokenValid = false;
+        }
+        finally
+        {
+            _tokenLock.Release();
+        }
     }
Also wrap GetPlainToken calls in ValidateTokenAsync and SaveAsync with the lock, and dispose _tokenLock in the Dispose method.

194-194: Sanitize exception messages displayed to users.

Line 194 exposes ex.Message directly in the UI, which could leak sensitive details or internal paths.

Apply this diff:
-            ValidationMessage = $"✗ Error: {ex.Message}";
+            ValidationMessage = "✗ Validation failed. Please check your connection and try again.";
The full exception is already logged on line 197 for debugging.

234-234: Sanitize exception message displayed to users.

Line 234 exposes ex.Message directly in the UI, which could leak sensitive information.

Apply this diff:
-            ValidationMessage = $"✗ Failed to save token: {ex.Message}";
+            ValidationMessage = "✗ Failed to save token. Please try again.";
The full exception is already logged on line 237 for debugging.
GenHub/GenHub/Features/GitHub/Views/GitHubTokenDialogView.axaml.cs (1)
24-30: Potential memory leak: Events not unwired when SetViewModel is called.

If SetViewModel is called multiple times, event handlers accumulate without being removed, preventing the ViewModel from being garbage collected. This issue was flagged in a previous review and remains present.

Apply this diff using named methods for proper cleanup:
+    private void OnSaveCompleted() => Close(true);
+    private void OnCancelRequested() => Close(false);
+
     public void SetViewModel(GitHubTokenDialogViewModel viewModel)
     {
+        if (DataContext is GitHubTokenDialogViewModel oldVm)
+        {
+            oldVm.SaveCompleted -= OnSaveCompleted;
+            oldVm.CancelRequested -= OnCancelRequested;
+        }
+
         DataContext = viewModel;
 
-        viewModel.SaveCompleted += () => Close(true);
-        viewModel.CancelRequested += () => Close(false);
+        viewModel.SaveCompleted += OnSaveCompleted;
+        viewModel.CancelRequested += OnCancelRequested;
     }

🧹 Nitpick comments (2)

GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/SharedViewModelModuleTests.cs (1)

99-105: Consider using directives instead of fully qualified names for cleaner code.

While the fully qualified names are functionally correct, adding using GenHub.Core.Interfaces.Shortcuts; and using GenHub.Core.Interfaces.GitHub; at the top of the file would improve readability.

Apply this diff to improve readability:

 using GenHub.Common.ViewModels;
 using GenHub.Core.Interfaces.Common;
 using GenHub.Core.Interfaces.Content;
+using GenHub.Core.Interfaces.GitHub;
 using GenHub.Core.Interfaces.Manifest;
+using GenHub.Core.Interfaces.Shortcuts;
 using GenHub.Core.Models.Common;

Then simplify the mock registrations:

-        // Mock IShortcutService to avoid dependency issues
-        var shortcutServiceMock = new Mock<GenHub.Core.Interfaces.Shortcuts.IShortcutService>();
-        services.AddSingleton<GenHub.Core.Interfaces.Shortcuts.IShortcutService>(shortcutServiceMock.Object);
-
-        // Mock IGitHubTokenStorage to avoid dependency issues
-        var tokenStorageMock = new Mock<GenHub.Core.Interfaces.GitHub.IGitHubTokenStorage>();
-        services.AddSingleton<GenHub.Core.Interfaces.GitHub.IGitHubTokenStorage>(tokenStorageMock.Object);
+        // Mock IShortcutService to avoid dependency issues
+        var shortcutServiceMock = new Mock<IShortcutService>();
+        services.AddSingleton<IShortcutService>(shortcutServiceMock.Object);
+
+        // Mock IGitHubTokenStorage to avoid dependency issues
+        var tokenStorageMock = new Mock<IGitHubTokenStorage>();
+        services.AddSingleton<IGitHubTokenStorage>(tokenStorageMock.Object);

GenHub/GenHub/Features/GitHub/Views/GitHubTokenDialogView.axaml.cs (1)

10-44: Consider implementing IDisposable for proper cleanup.

The View should implement IDisposable to ensure event handlers are unsubscribed when the window is closed or disposed. Currently, if the window closes unexpectedly (e.g., via the X button), the ViewModel's events remain subscribed.

Add IDisposable implementation:

-public partial class GitHubTokenDialogView : Window
+public partial class GitHubTokenDialogView : Window, IDisposable
 {
+    private GitHubTokenDialogViewModel? _viewModel;
+    private bool _disposed;
+
     public GitHubTokenDialogView()
     {
         InitializeComponent();
+        Closing += OnClosing;
     }
 
+    private void OnClosing(object? sender, WindowClosingEventArgs e)
+    {
+        Dispose();
+    }
+
+    public new void Dispose()
+    {
+        if (_disposed) return;
+        
+        if (_viewModel != null)
+        {
+            _viewModel.SaveCompleted -= OnSaveCompleted;
+            _viewModel.CancelRequested -= OnCancelRequested;
+        }
+        
+        _disposed = true;
+        base.Dispose();
+    }
+
+    private void OnSaveCompleted() => Close(true);
+    private void OnCancelRequested() => Close(false);
+
     public void SetViewModel(GitHubTokenDialogViewModel viewModel)
     {
+        if (_viewModel != null)
+        {
+            _viewModel.SaveCompleted -= OnSaveCompleted;
+            _viewModel.CancelRequested -= OnCancelRequested;
+        }
+
+        _viewModel = viewModel;
         DataContext = viewModel;
 
-        viewModel.SaveCompleted += () => Close(true);
-        viewModel.CancelRequested += () => Close(false);
+        _viewModel.SaveCompleted += OnSaveCompleted;
+        _viewModel.CancelRequested += OnCancelRequested;
     }

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3283582 and 4815bb5.

📒 Files selected for processing (30)

.github/workflows/ci.yml (5 hunks)
GenHub/Directory.Build.props (1 hunks)
GenHub/GenHub.Core/Constants/AppConstants.cs (5 hunks)
GenHub/GenHub.Core/Constants/BuildInfoConstants.cs (1 hunks)
GenHub/GenHub.Core/Constants/GitHubConstants.cs (1 hunks)
GenHub/GenHub.Core/Helpers/FileSizeFormatter.cs (1 hunks)
GenHub/GenHub.Core/Models/AppUpdate/ArtifactUpdateInfo.cs (1 hunks)
GenHub/GenHub.Core/Models/AppUpdate/PullRequestInfo.cs (1 hunks)
GenHub/GenHub.Core/Models/Common/UserSettings.cs (2 hunks)
GenHub/GenHub.Core/Models/Enums/UpdateChannel.cs (1 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Constants/AppConstantsTests.cs (1 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/Services/VelopackUpdateManagerTests.cs (12 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/ViewModels/UpdateNotificationViewModelTests.cs (4 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameProfiles/GameProcessManagerTests.cs (1 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/GameProfileModuleTests.cs (5 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/SharedViewModelModuleTests.cs (3 hunks)
GenHub/GenHub/Common/ViewModels/MainViewModel.cs (2 hunks)
GenHub/GenHub/Features/AppUpdate/Interfaces/IVelopackUpdateManager.cs (3 hunks)
GenHub/GenHub/Features/AppUpdate/Services/SimpleHttpServer.cs (1 hunks)
GenHub/GenHub/Features/AppUpdate/Services/VelopackUpdateManager.cs (7 hunks)
GenHub/GenHub/Features/AppUpdate/ViewModels/UpdateNotificationViewModel.cs (9 hunks)
GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationView.axaml (5 hunks)
GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationWindow.axaml (4 hunks)
GenHub/GenHub/Features/GameProfiles/Services/ProfileContentLoader.cs (5 hunks)
GenHub/GenHub/Features/GitHub/ViewModels/GitHubTokenDialogViewModel.cs (1 hunks)
GenHub/GenHub/Features/GitHub/Views/GitHubTokenDialogView.axaml (1 hunks)
GenHub/GenHub/Features/GitHub/Views/GitHubTokenDialogView.axaml.cs (1 hunks)
GenHub/GenHub/Features/Settings/ViewModels/SettingsViewModel.cs (8 hunks)
GenHub/GenHub/Features/Settings/Views/SettingsView.axaml (18 hunks)
GenHub/GenHub/Features/Settings/Views/SettingsView.axaml.cs (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (8)

GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/GameProfileModuleTests.cs
GenHub/GenHub.Core/Models/Common/UserSettings.cs
GenHub/GenHub/Features/AppUpdate/Services/SimpleHttpServer.cs
GenHub/GenHub/Features/Settings/Views/SettingsView.axaml.cs
GenHub/GenHub.Core/Constants/GitHubConstants.cs
GenHub/GenHub.Core/Constants/BuildInfoConstants.cs
GenHub/GenHub.Core/Helpers/FileSizeFormatter.cs
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameProfiles/GameProcessManagerTests.cs

🧰 Additional context used

🧬 Code graph analysis (7)

GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/ViewModels/UpdateNotificationViewModelTests.cs (3)

GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/SharedViewModelModuleTests.cs (1)

IUserSettingsService (160-171)

GenHub/GenHub.Core/Models/Common/UserSettings.cs (1)

UserSettings (7-148)

GenHub/GenHub/Features/AppUpdate/ViewModels/UpdateNotificationViewModel.cs (2)

UpdateNotificationViewModel (25-700)

UpdateNotificationViewModel (123-144)

GenHub/GenHub/Features/Settings/ViewModels/SettingsViewModel.cs (1)

GenHub/GenHub.Core/Constants/AppConstants.cs (1)

AppConstants (10-137)

GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/Services/VelopackUpdateManagerTests.cs (2)

GenHub/GenHub/Features/AppUpdate/Services/VelopackUpdateManager.cs (7)

HttpClient (934-950)

Task (93-184)

Task (187-298)

Task (301-513)

Task (516-560)

Task (624-847)

VelopackUpdateManager (30-951)

GenHub/GenHub/Features/AppUpdate/Interfaces/IVelopackUpdateManager.cs (5)

Task (21-21)

Task (29-29)

Task (37-37)

Task (46-46)

Task (109-109)

GenHub/GenHub.Tests/GenHub.Tests.Core/Constants/AppConstantsTests.cs (1)

GenHub/GenHub.Core/Constants/AppConstants.cs (1)

AppConstants (10-137)

GenHub/GenHub/Features/AppUpdate/ViewModels/UpdateNotificationViewModel.cs (3)

GenHub/GenHub/Features/AppUpdate/Interfaces/IVelopackUpdateManager.cs (5)

Task (21-21)

Task (29-29)

Task (37-37)

Task (46-46)

Task (109-109)

GenHub/GenHub/Features/AppUpdate/Services/VelopackUpdateManager.cs (5)

Task (93-184)

Task (187-298)

Task (301-513)

Task (516-560)

Task (624-847)

GenHub/GenHub/Infrastructure/Extensions/LoggerExtensions.cs (1)

LogError (46-51)

GenHub/GenHub/Features/GameProfiles/Services/ProfileContentLoader.cs (4)

GenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileSettingsViewModel.cs (2)

ObservableCollection (1279-1281)

ContentDisplayItem (1257-1272)

GenHub/GenHub.Core/Models/Content/ContentDisplayItem.cs (1)

ContentDisplayItem (11-234)

GenHub/GenHub.Core/Interfaces/Content/IContentDisplayFormatter.cs (2)

ContentDisplayItem (22-22)

ContentDisplayItem (32-36)

GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/Extensions/LoggerExtensionsTests.cs (1)

IsEnabled (144-144)

GenHub/GenHub/Features/GitHub/Views/GitHubTokenDialogView.axaml.cs (2)

GenHub/GenHub/Features/Settings/Views/SettingsView.axaml.cs (1)

InitializeComponent (76-79)

GenHub/GenHub/Features/GitHub/ViewModels/GitHubTokenDialogViewModel.cs (4)

GitHubTokenDialogViewModel (19-265)

GitHubTokenDialogViewModel (54-56)

GitHubTokenDialogViewModel (64-72)

SetToken (78-93)

🪛 actionlint (1.7.9)

.github/workflows/ci.yml

89-89: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

247-247: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

GitHub Check: Build Linux
GitHub Check: Build Windows

🔇 Additional comments (29)

GenHub/GenHub/Features/GameProfiles/Services/ProfileContentLoader.cs (2)

1-5: LGTM!

The using statements are properly organized and include all necessary namespaces.

495-495: Good improvement with named tuple elements.

The signature change from anonymous tuple to named tuple elements (Installation and Client) improves code readability and makes call sites more self-documenting.

GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationWindow.axaml (2)

9-9: LGTM: Window icon properly configured.

The Icon attribute is correctly set and references the same asset used elsewhere in the UI.

32-33: LGTM: Background gradient colors updated.

The gradient color change is appropriate and maintains sufficient contrast with the UI text elements.

GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/SharedViewModelModuleTests.cs (2)

118-118: LGTM! Addition aligns with PR objective.

The AddAppUpdateModule() registration correctly integrates the new Velopack update functionality into the DI test setup. The module placement in the registration order is appropriate.

27-139: LGTM! Test structure is comprehensive and well-organized.

The test effectively validates that all ViewModels can be resolved from the DI container. The mock setup, module registration order, and assertions are appropriate for this integration test.

.github/workflows/ci.yml (10)

96-124: LGTM! Build info extraction follows SemVer2 requirements.

The version format correctly produces SemVer2-compliant versions (e.g., 0.0.123 or 0.0.123-pr42) that meet Velopack's requirements. All necessary build metadata (SHORT_HASH, PR_NUMBER, VERSION, CHANNEL) is properly exported to GITHUB_OUTPUT for downstream use.

126-144: LGTM! Build metadata propagation is correct.

Build properties are properly constructed and passed to all .NET build commands using PowerShell splatting. This ensures version and build metadata flows through to the compiled assemblies.

145-162: LGTM! Publish step correctly propagates build metadata.

The Windows publish step properly passes all build properties to ensure the self-contained executable includes the correct version and metadata.

164-183: LGTM! Velopack packaging configuration is correct for Windows.

The Velopack packaging step properly uses the computed SemVer2 version and correctly specifies the Windows executable and .ico icon format (appropriate for Windows). Output artifacts are logged for verification.

202-224: LGTM! Artifact uploads are properly configured.

The three upload steps correctly capture all Velopack artifacts (update package, setup installer, and metadata) with version-stamped names and appropriate retention policies.

282-294: Build property propagation is correctly structured.

The Linux build step properly constructs and passes build metadata to .NET build commands. However, this depends on the CHANNEL export fix in the previous Extract Build Info step to function correctly.

295-305: Publish step structure is correct.

The Linux publish step properly passes build properties, though it also depends on the CHANNEL export fix to propagate correct metadata.

307-324: LGTM! Linux Velopack packaging uses correct formats.

The Linux packaging step properly uses PNG icon format (appropriate for AppImage), specifies the Linux executable without .exe extension, and references the computed SemVer2 version.

335-349: LGTM! Linux artifact uploads are appropriately configured.

The Linux upload steps correctly capture the update package and metadata using Velopack's Linux naming conventions (.nupkg and releases.*.json).

392-408: LGTM! Build summary correctly includes all job statuses.

The summary job properly aggregates results from all build and release jobs, providing a clear status overview regardless of individual job outcomes.

GenHub/GenHub.Core/Models/Enums/UpdateChannel.cs (1)

1-22: LGTM! Well-documented enum for update channels.

The enum is clearly defined with appropriate XML documentation for each member. The three update channels (Stable, Prerelease, Artifacts) align well with the PR's objectives for supporting multiple update sources.

GenHub/GenHub/Common/ViewModels/MainViewModel.cs (1)

355-379: LGTM! Clean implementation of update dismissal logic.

The implementation properly handles version comparison and user dismissal state. The case-insensitive string comparison and null checks are appropriate for version matching.

GenHub/GenHub.Tests/GenHub.Tests.Core/Constants/AppConstantsTests.cs (1)

24-31: LGTM! Tests properly updated for new versioning scheme.

The assertions correctly validate the new 0.0.x alpha version format and DisplayVersion prefix. The addition of a null check for DisplayVersion is a good defensive practice.

GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/ViewModels/UpdateNotificationViewModelTests.cs (1)

1-1: LGTM! Tests properly updated for new dependency.

The addition of IUserSettingsService mock is consistent across all test methods, and the mock setup appropriately returns a default UserSettings instance.

Also applies to: 25-31, 45-51, 65-71

GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/Services/VelopackUpdateManagerTests.cs (2)

1-2: LGTM! Proper mock setup for new dependencies.

The addition of IGitHubTokenStorage and IUserSettingsService mocks is consistent across the test class, with appropriate default behavior configured.

Also applies to: 16-17, 26-27, 32-36

172-189: LGTM! Good test coverage for PAT-less artifact updates.

The new test properly validates that artifact update checks return null when no PAT is available, covering an important edge case.

GenHub/GenHub/Features/Settings/Views/SettingsView.axaml (1)

452-575: LGTM! Comprehensive Updates section with PAT management.

The new Updates section provides a well-structured UI for update channel selection, PAT management, and artifact browsing. The layout and organization are clear and user-friendly.

GenHub/GenHub/Features/AppUpdate/Interfaces/IVelopackUpdateManager.cs (1)

1-110: LGTM! Well-designed interface expansion.

The interface cleanly extends the update manager with PR artifact capabilities. The new methods and properties are well-documented and logically organized.

GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationView.axaml (1)

38-290: Layout refactoring resolves past Grid.Row overlap concerns.

The past review flagged potential overlap of multiple elements in Grid.Row="2". The current implementation has been refactored to use a ScrollViewer with a StackPanel (lines 39-290), which ensures sections stack vertically without overlap. This is a better approach than the mutually-exclusive visibility flags suggested in the past review.

GenHub/GenHub.Core/Constants/AppConstants.cs (1)

27-94: LGTM! Clean metadata extraction and display formatting.

The Lazy initialization pattern for build metadata is appropriate, and the FullDisplayVersion logic provides clear, context-aware version strings for different build types.

GenHub/Directory.Build.props (1)

1-44: LGTM! Clean MSBuild metadata integration.

The build properties correctly embed CI metadata into assembly attributes for runtime access. The conditional InformationalVersion ensures CI builds include git hash while local builds remain clean.

GenHub/GenHub/Features/GitHub/Views/GitHubTokenDialogView.axaml (1)

1-170: LGTM! Well-structured dialog UI with proper bindings.

The XAML implementation is clean and well-organized:

Proper data binding to ViewModel properties

IsEnabled bindings prevent invalid actions (Save button requires IsTokenValid, buttons disabled during validation)

Good UX with loading indicators, validation messages, and color-coded feedback

Accessible design with tooltips and proper focus management

Previous issues have been addressed (Save button IsEnabled binding).

GenHub/GenHub/Features/GitHub/Views/GitHubTokenDialogView.axaml.cs (1)

32-38: LGTM! Proper type checking and null handling.

The OnTokenPasswordChanged method correctly validates the sender type and DataContext before calling SetToken, preventing potential runtime errors.

coderabbitai · 2025-12-16T21:16:10Z

.github/workflows/ci.yml

      - name: Cache NuGet Packages
        uses: actions/cache@v3
        with:
          path: ~/.nuget/packages
          key: ${{ runner.os }}-nuget-${{ hashFiles('**/*.csproj') }}
          restore-keys: |
            ${{ runner.os }}-nuget-
-      
+


⚠️ Potential issue | 🔴 Critical

Critical: actions/cache@v3 must be updated to v4.

Both NuGet cache steps still use the outdated actions/cache@v3, despite past comments indicating this was addressed. Actionlint confirms v3 runners are no longer supported on GitHub Actions. The workflow already uses v4 for other actions (checkout, setup-dotnet, upload-artifact).

Apply this diff to both cache steps:

- name: Cache NuGet Packages - uses: actions/cache@v3 + uses: actions/cache@v4 with: path: ~/.nuget/packages key: ${{ runner.os }}-nuget-${{ hashFiles('**/*.csproj') }} restore-keys: | ${{ runner.os }}-nuget-

Also applies to: 246-253

🧰 Tools

🪛 actionlint (1.7.9)

89-89: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

🤖 Prompt for AI Agents

.github/workflows/ci.yml around lines 88-95 (and similarly at 246-253): the workflow uses actions/cache@v3 for NuGet caching which is deprecated; update both cache steps to use actions/cache@v4 by changing the uses line to "uses: actions/cache@v4" while keeping the existing with: path, key and restore-keys settings unchanged so the cache behavior remains the same.

coderabbitai · 2025-12-16T21:16:10Z

GenHub/GenHub/Features/GitHub/ViewModels/GitHubTokenDialogViewModel.cs

+    private string GetPlainToken()
+    {
+        if (_secureToken == null)
+            return string.Empty;
+
+        var ptr = System.Runtime.InteropServices.Marshal.SecureStringToGlobalAllocUnicode(_secureToken);
+        try
+        {
+            return System.Runtime.InteropServices.Marshal.PtrToStringUni(ptr) ?? string.Empty;
+        }
+        finally
+        {
+            System.Runtime.InteropServices.Marshal.ZeroFreeGlobalAllocUnicode(ptr);
+        }
+    }


⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# Search for platform detection/guards and project configuration cd /tmp && find . -name "*.csproj" -o -name "*.vbproj" 2>/dev/null | head -20

Repository: undead2146/GenHub

Length of output: 43

🏁 Script executed:

# Find project files and check target frameworks find . -name "*.csproj" -o -name "*.vbproj" | head -10 # Search for the specific file mentioned in the review find . -name "GitHubTokenDialogViewModel.cs" -type f # Look for platform detection/guards rg -n "RuntimeInformation|OperatingSystem\.Is|if.*Windows|#if.*WIN" --type cs | head -30 # Check around the GetPlainToken method for platform guards or conditional compilation rg -n -B5 -A10 "GetPlainToken|SecureStringToGlobalAllocUnicode" --type cs

Repository: undead2146/GenHub

Length of output: 13197

Add platform guards to GetPlainToken and other shared SecureString conversions, or move to platform-specific implementations.

The GetPlainToken method uses Marshal.SecureStringToGlobalAllocUnicode, a Windows-only Win32 API. Since the application explicitly targets both Windows and Linux (evidenced by platform-specific project folders), this code will fail at runtime on non-Windows platforms. Add an OperatingSystem.IsWindows() guard or throw a PlatformNotSupportedException, or relocate this method to a Windows-specific implementation similar to WindowsGitHubTokenStorage. This pattern also affects OctokitGitHubApiClient.cs (line 474) and VelopackUpdateManager.cs (line 920).

🤖 Prompt for AI Agents

In GenHub/GenHub/Features/GitHub/ViewModels/GitHubTokenDialogViewModel.cs around lines 250-264, the method uses Marshal.SecureStringToGlobalAllocUnicode (Win32-only) and will fail on non-Windows platforms; update this by either moving the method into a Windows-only implementation (e.g., alongside WindowsGitHubTokenStorage) or add an OperatingSystem.IsWindows() guard that performs the Marshal call only on Windows and throws a PlatformNotSupportedException on other OSes, and ensure the pointer is only freed if non-zero in the finally; apply the same platform-guard or relocation pattern to the similar conversions in OctokitGitHubApiClient.cs (around line 474) and VelopackUpdateManager.cs (around line 920).

coderabbitai

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

GenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameProfiles/GameProcessManagerTests.cs (1)
47-58: Rename test method to match its expected behavior.

The method name TerminateProcessAsync_WithNonExistentProcessId_ShouldReturnFailure contradicts the actual behavior being tested. The test correctly validates idempotent termination (success for non-existent processes), but the name suggests it should return failure.

Apply this diff to fix the method name:
-    public async Task TerminateProcessAsync_WithNonExistentProcessId_ShouldReturnFailure()
+    public async Task TerminateProcessAsync_WithNonExistentProcessId_ShouldReturnSuccess()

♻️ Duplicate comments (31)

GenHub/GenHub.Core/Models/AppUpdate/PullRequestInfo.cs (2)
11-31: Fix modifier order: public required instead of required public.

Static analysis (SA1206) correctly identifies that the accessibility modifier should precede required. This is the idiomatic C# modifier ordering.
-    required public int Number { get; init; }
+    public required int Number { get; init; }
...
-    required public string Title { get; init; }
+    public required string Title { get; init; }
...
-    required public string BranchName { get; init; }
+    public required string BranchName { get; init; }
...
-    required public string Author { get; init; }
+    public required string Author { get; init; }
...
-    required public string State { get; init; }
+    public required string State { get; init; }
53-56: Use null-safe string comparison for IsOpen.

Although State is required, it could still be null at runtime from faulty deserialization. State.Equals(...) will throw NullReferenceException in that case.
-    public bool IsOpen => State.Equals("open", StringComparison.OrdinalIgnoreCase);
+    public bool IsOpen => string.Equals(State, "open", StringComparison.OrdinalIgnoreCase);
GenHub/GenHub/Features/GameProfiles/Services/ProfileContentLoader.cs (2)
231-251: Incomplete property cloning may cause data loss.

This helper only copies a subset of ContentDisplayItem properties. Properties like ReleaseDate, FileSize, IconUrl, IsInstalled, IsAvailable, and others will be lost if the source items have them populated.

Consider adding the missing properties:
             GameClientId = item.GameClientId,
             IsEnabled = enabledIds.Contains(item.ManifestId),
+            ReleaseDate = item.ReleaseDate,
+            FileSize = item.FileSize,
+            IconUrl = item.IconUrl,
+            IsInstalled = item.IsInstalled,
+            IsAvailable = item.IsAvailable,
         }));
400-405: Double enumeration of LINQ query.

items.Count() enumerates the IEnumerable, then new ObservableCollection<ContentDisplayItem>(items) enumerates it again. Materialize the collection first.
     var items = manifestsResult.Data
         .Where(m => m.ContentType == contentType)
-        .Select(m => CreateManifestDisplayItem(m, isEnabled: enabledIds.Contains(m.Id.Value)));
+        .Select(m => CreateManifestDisplayItem(m, isEnabled: enabledIds.Contains(m.Id.Value)))
+        .ToList();

-    logger.LogInformation("Loaded {Count} content items for type {ContentType}", items.Count(), contentType);
+    logger.LogInformation("Loaded {Count} content items for type {ContentType}", items.Count, contentType);
     return new ObservableCollection<ContentDisplayItem>(items);
GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationWindow.axaml (1)
50-52: Mark the decorative icon as non-interactive for accessibility.

The Image element should be marked as decorative to prevent screen readers from treating it separately from the accompanying "GenHub Updates" text label.
 <Image Source="/Assets/Icons/generalshub-icon.png"
        Width="28" Height="28"
-       Margin="8,0,0,0" />
+       Margin="8,0,0,0"
+       AutomationProperties.AccessibilityView="Raw" />
GenHub/GenHub/Features/AppUpdate/Services/SimpleHttpServer.cs (4)
34-43: Validate port before constructing the listener prefix.

This avoids hard-to-diagnose HttpListenerExceptions for invalid values.
 public SimpleHttpServer(string nupkgPath, string releasesPath, int port, ILogger logger)
 {
+    if (port is < 1 or > 65535)
+        throw new ArgumentOutOfRangeException(nameof(port), "Port must be 1..65535.");
     _nupkgPath = nupkgPath ?? throw new ArgumentNullException(nameof(nupkgPath));
64-84: Stop() leaves the server non-restartable.

Start() checks _serverTask != null but Stop() never resets _serverTask/_cts, so calling Start() after Stop() is a no-op.
     _logger.LogInformation("HTTP server stopped");
+
+    _serverTask = null;
+    _cts.Dispose();
+    _cts = null;
 }
101-101: Fire-and-forget task may swallow exceptions.

_ = Task.Run(...) discards the task, so exceptions thrown in ProcessRequestAsync are unobserved.
-                _ = Task.Run(() => ProcessRequestAsync(context), cancellationToken);
+                var requestTask = Task.Run(() => ProcessRequestAsync(context), cancellationToken);
+                _ = requestTask.ContinueWith(t =>
+                {
+                    if (t.IsFaulted && t.Exception != null)
+                        _logger.LogError(t.Exception, "Unhandled error in request processing");
+                }, TaskScheduler.Default);
114-172: Ensure the HTTP response is always closed (even on exceptions).

If an exception occurs before line 166, the response remains open; clients can hang.
 private async Task ProcessRequestAsync(HttpListenerContext context)
 {
+    var response = context.Response;
     try
     {
         var request = context.Request;
-        var response = context.Response;
         // ... existing code ...
-        response.Close();
     }
     catch (Exception ex)
     {
         _logger.LogError(ex, "Error processing HTTP request");
     }
+    finally
+    {
+        try { response.Close(); } catch { /* best-effort */ }
+    }
 }
GenHub/GenHub.Core/Models/AppUpdate/ArtifactUpdateInfo.cs (2)
72-84: Guard against null/empty Version and GitHash in DisplayVersion.

The DisplayVersion property assumes Version is non-null when calling Split('+'). Additionally, if GitHash is empty or whitespace, the display will show awkward formatting like v1.0.0 ().

This was flagged in a previous review. Consider applying a fix like:
     public string DisplayVersion
     {
         get
         {
+            var hashPart = string.IsNullOrWhiteSpace(GitHash) ? string.Empty : $" ({GitHash})";
             if (PullRequestNumber.HasValue)
             {
-                // Strip any build metadata from version (everything after +)
-                var baseVersion = Version.Split('+')[0];
-                return $"v{baseVersion} ({GitHash})";
+                var baseVersion = Version?.Split('+')[0] ?? "0.0.0";
+                return $"v{baseVersion}{hashPart}";
             }
 
-            return $"v{Version} ({GitHash})";
+            return $"v{Version}{hashPart}";
         }
     }
22-22: Consider using DateTimeOffset for CreatedAt.

GitHub API timestamps are UTC. Using DateTimeOffset instead of DateTime avoids DateTimeKind ambiguity and makes timezone handling explicit.

This was noted in a previous review.

Also applies to: 62-62
.github/workflows/ci.yml (3)
88-95: Update actions/cache from v3 to v4.

Both NuGet cache steps still use actions/cache@v3. Static analysis confirms v3 runners are no longer supported on GitHub Actions. The workflow already uses v4 for other actions.

Apply this diff to both cache steps:
       - name: Cache NuGet Packages
-        uses: actions/cache@v3
+        uses: actions/cache@v4
         with:
           path: ~/.nuget/packages
Also applies to: 246-253

386-387: Parameterize the release tag to use the computed build version.

The release tag is hardcoded as v0.0.${{ github.run_number }}, duplicating version logic from the build jobs. If the version computation changes, these will diverge.

Expose VERSION as a job output from build-windows and reference it here:
# In build-windows job:
    outputs:
      version: ${{ steps.buildinfo.outputs.VERSION }}

# In release job:
          tag_name: v${{ needs.build-windows.outputs.version }}
          name: GenHub ${{ needs.build-windows.outputs.version }}
254-281: Export CHANNEL to GitHub Output on Linux.

The Linux Extract Build Info step computes CHANNEL (lines 265-268) but does not export it to $GITHUB_OUTPUT. Only SHORT_HASH, PR_NUMBER, and VERSION are exported (lines 278-280). If downstream steps reference ${{ steps.buildinfo.outputs.CHANNEL }}, they will receive an empty value.

Add the missing export after line 280:
           echo "SHORT_HASH=$SHORT_HASH" >> $GITHUB_OUTPUT
           echo "PR_NUMBER=$PR_NUMBER" >> $GITHUB_OUTPUT
           echo "VERSION=$VERSION" >> $GITHUB_OUTPUT
+          echo "CHANNEL=$CHANNEL" >> $GITHUB_OUTPUT
GenHub/GenHub/Features/Settings/Views/SettingsView.axaml (3)
496-498: PatStatusColor binding requires a Brush type, not a string.

Ellipse.Fill expects a Brush type, but PatStatusColor appears to return a hex string (e.g., "#4CAF50"). This will cause a binding error at runtime.

Either change the ViewModel property to return SolidColorBrush:
public SolidColorBrush PatStatusColor => HasGitHubPat 
    ? new SolidColorBrush(Color.Parse("#4CAF50")) 
    : new SolidColorBrush(Color.Parse("#FF5252"));
Or use a converter in the binding.

570-572: Fix IsVisible binding - negation on integer doesn't work as intended.

The binding IsVisible="{Binding !AvailableArtifacts.Count}" attempts to negate an integer, which will likely produce binding errors. When Count is 0, negation should yield true, but this syntax is invalid in Avalonia.

Add a boolean property to the ViewModel:
public bool HasNoArtifacts => AvailableArtifacts?.Count == 0;
Then update the binding:
-           IsVisible="{Binding !AvailableArtifacts.Count}"
+           IsVisible="{Binding HasNoArtifacts}"
222-223: Fix formatting inconsistencies in description text.

There's a double space before the opening parenthesis and a missing space after the comma.
-            <TextBlock Text="Timeout for each download operation  (10-3600 seconds,default: 600 seconds)"
+            <TextBlock Text="Timeout for each download operation (10-3600 seconds, default: 600 seconds)"
GenHub/GenHub/Features/Settings/ViewModels/SettingsViewModel.cs (4)
698-736: LoadPatStatusAsync conflates "token exists" with "token valid".

Setting IsPatValid = true solely because HasGitHubPat is true (line 713) means expired or revoked tokens will show a green "valid" state. Consider keeping IsPatValid = false by default and only setting it to true after an explicit validation call succeeds.

768-785: PAT validation logic always succeeds after saving.

The token is saved at line 768 before validation completes. Then the condition artifact != null || _gitHubTokenStorage.HasToken() (line 778) will always be true since HasToken() returns true after saving—even if the PAT is unauthorized or lacks required scopes. This allows invalid PATs to show "PAT validated successfully ✓".

Remove the || _gitHubTokenStorage.HasToken() clause and ensure proper rollback on validation failure:
                     var artifact = await _updateManager.CheckForArtifactUpdatesAsync();
-                    if (artifact != null || _gitHubTokenStorage.HasToken())
+                    if (artifact != null)
                     {
                         PatStatusMessage = "PAT validated successfully ✓";
                         IsPatValid = true;
                         HasGitHubPat = true;
                         GitHubPatInput = string.Empty;
                         return;
                     }
+                    // No artifacts found - cannot fully validate scopes
+                    PatStatusMessage = "PAT saved but could not verify scopes";
+                    IsPatValid = false;
+                    HasGitHubPat = _gitHubTokenStorage.HasToken();
+                    return;
875-884: Marshal AvailableArtifacts mutations to the UI thread.

AvailableArtifacts.Clear() (line 876) and AvailableArtifacts.Add(artifact) (line 883) can execute off the UI thread after awaits, which can crash Avalonia-bound collections.
         IsLoadingArtifacts = true;
-        AvailableArtifacts.Clear();
+        await Dispatcher.UIThread.InvokeAsync(() => AvailableArtifacts.Clear());
 
         try
         {
             var artifact = await _updateManager.CheckForArtifactUpdatesAsync();
             if (artifact != null)
             {
-                AvailableArtifacts.Add(artifact);
+                await Dispatcher.UIThread.InvokeAsync(() => AvailableArtifacts.Add(artifact));
                 PatStatusMessage = $"Found {AvailableArtifacts.Count} artifact(s)";
             }
905-927: Persist the channel setting to disk after updating.

_userSettingsService.Update(...) (line 916) only modifies settings in-memory. Per the interface, SaveAsync() must be called to write changes to disk. The update channel selection will be lost on restart.
         // Persist setting
         _userSettingsService.Update(s => s.UpdateChannel = value);
+        _ = _userSettingsService.SaveAsync();
GenHub/GenHub/Features/AppUpdate/ViewModels/UpdateNotificationViewModel.cs (5)
142-167: InitializeAsync lacks exception handling and is fire-and-forget.

The constructor discards the task (line 143) and InitializeAsync has no try-catch. If LoadPullRequestsAsync or CheckForUpdatesAsync throws, the exception will be unobserved.

Wrap the method body in try-catch:
 private async Task InitializeAsync()
 {
+    try
+    {
         // Load subscribed PR from settings
         var settings = _userSettingsService.Get();
         if (settings.SubscribedPrNumber.HasValue)
         {
             _velopackUpdateManager.SubscribedPrNumber = settings.SubscribedPrNumber;
             _logger.LogInformation("Loaded subscribed PR #{PrNumber} from settings", settings.SubscribedPrNumber);
         }
 
         // Load PRs FIRST so SubscribedPr object is populated before update check
         if (HasPat)
         {
             await LoadPullRequestsAsync();
         }
 
         // Now check for updates - SubscribedPr will be properly populated
         await CheckForUpdatesAsync();
+    }
+    catch (Exception ex)
+    {
+        _logger.LogError(ex, "Initialization failed");
+        HasError = true;
+        ErrorMessage = $"Initialization failed: {ex.Message}";
+        StatusMessage = "Initialization failed";
+    }
 }
545-551: Fire-and-forget SaveAsync() discards errors.

Line 549 uses _ = _userSettingsService.SaveAsync() which silently drops any save failures. The user won't know their dismissed version wasn't persisted.
         _userSettingsService.Update(s => s.DismissedUpdateVersion = LatestVersion);
-        _ = _userSettingsService.SaveAsync();
+        _ = _userSettingsService.SaveAsync().ContinueWith(t =>
+        {
+            if (t.IsFaulted)
+                _logger.LogError(t.Exception, "Failed to save dismissed version");
+        }, TaskScheduler.Default);
611-612: Marshal AvailablePullRequests.Clear() to the UI thread.

AvailablePullRequests.Clear() on line 612 runs outside the dispatcher, but the subsequent Add loop (lines 620-626) is correctly wrapped. Both mutations should be on the UI thread for consistency.
         IsLoadingPullRequests = true;
-        AvailablePullRequests.Clear();
+        await Dispatcher.UIThread.InvokeAsync(() => AvailablePullRequests.Clear());
666-669: Fire-and-forget SaveAsync in SubscribeToPr.

Same pattern as DismissUpdate—errors are silently dropped.
         _userSettingsService.Update(s => s.SubscribedPrNumber = prNumber);
-        _ = _userSettingsService.SaveAsync();
+        _ = _userSettingsService.SaveAsync().ContinueWith(t =>
+        {
+            if (t.IsFaulted)
+                _logger.LogError(t.Exception, "Failed to save PR subscription");
+        }, TaskScheduler.Default);
694-697: Fire-and-forget SaveAsync in UnsubscribeFromPr.

Same pattern—save errors are discarded.
GenHub/GenHub/Features/AppUpdate/Services/VelopackUpdateManager.cs (5)
62-79: Remove debug logging from hot property getters.

HasUpdateAvailableFromGitHub and LatestVersionFromGitHub are likely polled frequently by UI bindings. Logging on every access will spam logs and add overhead.
     public bool HasUpdateAvailableFromGitHub
     {
         get
         {
-            logger.LogDebug("HasUpdateAvailableFromGitHub property accessed: {Value}", _hasUpdateFromGitHub);
             return _hasUpdateFromGitHub;
         }
     }

     public string? LatestVersionFromGitHub
     {
         get
         {
-            logger.LogDebug("LatestVersionFromGitHub property accessed: '{Value}'", _latestVersionFromGitHub ?? "NULL");
             return _latestVersionFromGitHub;
         }
     }
201-210: Dispose SecureString after use.

LoadTokenAsync returns a SecureString which implements IDisposable. The token should be disposed after use to clear sensitive data from memory promptly.
-            var token = await gitHubTokenStorage.LoadTokenAsync();
+            using var token = await gitHubTokenStorage.LoadTokenAsync();
             if (token == null)
             {
                 logger.LogWarning("Failed to load GitHub PAT");
                 return null;
             }
Apply the same fix at line 319 in GetOpenPullRequestsAsync and line 643 in InstallPrArtifactAsync.

217-222: Distinguish 401/403 auth failures from empty results.

Both CheckForArtifactUpdatesAsync and GetOpenPullRequestsAsync return null/empty for any HTTP error, including 401/403 unauthorized responses. This makes authentication failures indistinguishable from legitimate empty states, breaking PAT validation logic in SettingsViewModel (where artifact != null || HasToken() incorrectly marks invalid PATs as valid).

Consider checking for 401/403 explicitly and throwing or returning a richer result type:
             if (!runsResponse.IsSuccessStatusCode)
             {
+                if (runsResponse.StatusCode == System.Net.HttpStatusCode.Unauthorized ||
+                    runsResponse.StatusCode == System.Net.HttpStatusCode.Forbidden)
+                {
+                    throw new UnauthorizedAccessException($"GitHub API returned {runsResponse.StatusCode}. Check PAT scopes.");
+                }
                 logger.LogWarning("Failed to fetch workflow runs: {Status}", runsResponse.StatusCode);
                 return null;
             }
373-468: N+1 GitHub API pattern persists—fetches workflow runs inside PR loop.

For each open PR (line 351), the code fetches workflow runs (lines 376-378) and then artifacts. With 30 PRs, this makes 30+ API calls, quickly exhausting rate limits.

Fetch workflow runs once before the PR loop, group by PR number, then look up runs for each PR:
// Before the PR loop, fetch all recent successful runs once
var runsUrl = $"https://api.github.com/repos/{owner}/{repo}/actions/runs?status=success&per_page=100";
var runsResponse = await client.GetAsync(runsUrl, cancellationToken);

Dictionary<int, JsonElement> runsByPr = new();
if (runsResponse.IsSuccessStatusCode)
{
    var runsJson = await runsResponse.Content.ReadAsStringAsync(cancellationToken);
    var runsData = JsonSerializer.Deserialize<JsonElement>(runsJson);
    if (runsData.TryGetProperty("workflow_runs", out var runs))
    {
        foreach (var run in runs.EnumerateArray())
        {
            if (run.TryGetProperty("pull_requests", out var prs) && prs.GetArrayLength() > 0)
            {
                var prNum = prs[0].GetProperty("number").GetInt32();
                if (!runsByPr.ContainsKey(prNum))
                    runsByPr[prNum] = run; // Keep first (most recent)
            }
        }
    }
}

// Inside PR loop, look up from dictionary instead of making API call
if (runsByPr.TryGetValue(prNumber, out var run))
{
    // Fetch artifacts only for this run
    ...
}
884-891: Minor TOCTOU race in FindAvailablePort.

The port is released immediately after listener.Stop(), creating a window where another process could claim it before the HTTP server binds. This is a minor edge case, but for robustness consider keeping the listener and passing it to the HTTP server, or using retry logic if binding fails.

🧹 Nitpick comments (7)

GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/GameProfileModuleTests.cs (1)
260-260: Consider adding a using directive for consistency.

The fully qualified namespace GenHub.Core.Interfaces.Shortcuts.IShortcutService is inconsistent with other interface references in this test (e.g., IGameInstallationService, IContentManifestPool). Adding using GenHub.Core.Interfaces.Shortcuts; at the top would improve readability.

Apply this diff to add the using directive:
 using GenHub.Core.Interfaces.Storage;
+using GenHub.Core.Interfaces.Shortcuts;
 using GenHub.Core.Interfaces.Workspace;
Then simplify line 260:
-        services.AddSingleton<GenHub.Core.Interfaces.Shortcuts.IShortcutService>(new Mock<GenHub.Core.Interfaces.Shortcuts.IShortcutService>().Object);
+        services.AddSingleton<IShortcutService>(new Mock<IShortcutService>().Object);
GenHub/GenHub/Common/ViewModels/MainViewModel.cs (1)
379-388: Add blank line before the comment to satisfy StyleCop (SA1515, SA1513).

Static analysis flagged missing blank lines around the second condition block.
                     _logger?.LogDebug("Update {Version} was dismissed by user, hiding notification", latestVersionBase);
                     HasUpdateAvailable = false;
                 }
+
                 // Also check if we're already on this version (ignoring build metadata)
                 else if (!string.IsNullOrEmpty(latestVersionBase) &&
                          string.Equals(latestVersionBase, currentVersionBase, StringComparison.OrdinalIgnoreCase))
GenHub/GenHub/Features/AppUpdate/Interfaces/IVelopackUpdateManager.cs (1)

91-94: Consider adding property change notification for SubscribedPrNumber.

Since SubscribedPrNumber is a settable property that likely affects UI state (e.g., IsPrMergedOrClosed depends on it), implementations should raise PropertyChanged when this value changes. The interface itself doesn't require INotifyPropertyChanged, but consider documenting this expectation or adding an event.

This is a design consideration - if the implementing class already implements INotifyPropertyChanged and raises events for these properties, no changes are needed.
GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationView.axaml (1)
246-246: Consider adding explicit xmlns for ObjectConverters for clarity.

The binding at line 246 uses ObjectConverters.IsNotNull without an explicit namespace prefix. While this works in Avalonia (implicit namespace resolution is supported), explicitly importing the converters namespace makes the binding more maintainable and avoids potential parser ambiguity:
 <UserControl xmlns="https://github.com/avaloniaui"
     xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
     xmlns:vm="using:GenHub.Features.AppUpdate.ViewModels"
+    xmlns:dc="clr-namespace:Avalonia.Data.Converters;assembly=Avalonia.Base"
Then update:
-    IsVisible="{Binding SubscribedPr, Converter={x:Static ObjectConverters.IsNotNull}}"
+    IsVisible="{Binding SubscribedPr, Converter={x:Static dc:ObjectConverters.IsNotNull}}"
GenHub/GenHub/Features/Settings/ViewModels/SettingsViewModel.cs (1)
919-922: Fire-and-forget LoadArtifactsAsync may swallow exceptions.

Line 921 discards the task with _ = LoadArtifactsAsync(). If the method throws, the exception will be unobserved. Consider adding a continuation to log failures:
         if (value == UpdateChannel.Artifacts && HasGitHubPat)
         {
-            _ = LoadArtifactsAsync();
+            _ = LoadArtifactsAsync().ContinueWith(t =>
+            {
+                if (t.IsFaulted && t.Exception != null)
+                    _logger.LogError(t.Exception, "Failed to load artifacts");
+            }, TaskScheduler.Default);
         }
GenHub/GenHub.Core/Constants/AppConstants.cs (1)
129-136: Consider caching the assembly reference.

GetAssemblyMetadata calls Assembly.GetExecutingAssembly() on each invocation. While the Lazy wrappers ensure this only happens once per property, if this helper were used more broadly, caching the assembly reference would reduce reflection overhead.
+    private static readonly Lazy<Assembly> _executingAssembly = new(() => Assembly.GetExecutingAssembly());
+
     private static string? GetAssemblyMetadata(string key)
     {
-        var assembly = Assembly.GetExecutingAssembly();
+        var assembly = _executingAssembly.Value;
         return assembly
             .GetCustomAttributes<AssemblyMetadataAttribute>()
             .FirstOrDefault(a => a.Key == key)
             ?.Value;
     }
GenHub/GenHub/Features/AppUpdate/Services/VelopackUpdateManager.cs (1)
803-822: Post-restart handling may race with server disposal.

After calling ApplyUpdatesAndRestart (line 807), the code waits 5 seconds then throws if the app didn't exit. However, the finally block will dispose the HTTP server (line 833), potentially while Velopack is still downloading from it. ApplyUpdatesAndRestart may not block until the download completes.

Consider keeping the server alive longer or not throwing if the method returns (since Velopack stages the update for next launch):
                 localUpdateManager.ApplyUpdatesAndRestart(updateInfo.TargetFullRelease);
 
-                // If we get here, the exit might not have happened immediately
-                logger.LogWarning("ApplyUpdatesAndExit returned without exiting - waiting for exit...");
-                await Task.Delay(5000, cancellationToken);
-
-                logger.LogError("Application did not exit after ApplyUpdatesAndExit. Update may have failed.");
-                throw new InvalidOperationException("Application did not exit after applying update");
+                // ApplyUpdatesAndRestart should not return - if it does, update is staged
+                logger.LogWarning("ApplyUpdatesAndRestart returned - update staged for next launch");

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4815bb5 and a28a068.

📒 Files selected for processing (31)

.github/workflows/ci.yml (5 hunks)
GenHub/Directory.Build.props (1 hunks)
GenHub/GenHub.Core/Constants/AppConstants.cs (5 hunks)
GenHub/GenHub.Core/Constants/BuildInfoConstants.cs (1 hunks)
GenHub/GenHub.Core/Constants/GitHubConstants.cs (1 hunks)
GenHub/GenHub.Core/Helpers/FileSizeFormatter.cs (2 hunks)
GenHub/GenHub.Core/Models/AppUpdate/ArtifactUpdateInfo.cs (1 hunks)
GenHub/GenHub.Core/Models/AppUpdate/PullRequestInfo.cs (1 hunks)
GenHub/GenHub.Core/Models/Common/UserSettings.cs (2 hunks)
GenHub/GenHub.Core/Models/Enums/UpdateChannel.cs (1 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Constants/AppConstantsTests.cs (1 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/Services/VelopackUpdateManagerTests.cs (12 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/ViewModels/UpdateNotificationViewModelTests.cs (4 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameProfiles/GameProcessManagerTests.cs (1 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/GameProfileModuleTests.cs (5 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/SharedViewModelModuleTests.cs (3 hunks)
GenHub/GenHub/Common/ViewModels/MainViewModel.cs (2 hunks)
GenHub/GenHub/Common/Views/MainView.axaml (2 hunks)
GenHub/GenHub/Features/AppUpdate/Interfaces/IVelopackUpdateManager.cs (3 hunks)
GenHub/GenHub/Features/AppUpdate/Services/SimpleHttpServer.cs (1 hunks)
GenHub/GenHub/Features/AppUpdate/Services/VelopackUpdateManager.cs (7 hunks)
GenHub/GenHub/Features/AppUpdate/ViewModels/UpdateNotificationViewModel.cs (9 hunks)
GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationView.axaml (5 hunks)
GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationWindow.axaml (4 hunks)
GenHub/GenHub/Features/GameProfiles/Services/ProfileContentLoader.cs (5 hunks)
GenHub/GenHub/Features/GitHub/ViewModels/GitHubTokenDialogViewModel.cs (1 hunks)
GenHub/GenHub/Features/GitHub/Views/GitHubTokenDialogView.axaml (1 hunks)
GenHub/GenHub/Features/GitHub/Views/GitHubTokenDialogView.axaml.cs (1 hunks)
GenHub/GenHub/Features/Settings/ViewModels/SettingsViewModel.cs (8 hunks)
GenHub/GenHub/Features/Settings/Views/SettingsView.axaml (18 hunks)
GenHub/GenHub/Features/Settings/Views/SettingsView.axaml.cs (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (10)

GenHub/GenHub/Features/Settings/Views/SettingsView.axaml.cs
GenHub/GenHub.Tests/GenHub.Tests.Core/Constants/AppConstantsTests.cs
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/ViewModels/UpdateNotificationViewModelTests.cs
GenHub/GenHub.Core/Constants/BuildInfoConstants.cs
GenHub/GenHub.Core/Models/Common/UserSettings.cs
GenHub/GenHub/Features/GitHub/ViewModels/GitHubTokenDialogViewModel.cs
GenHub/GenHub/Features/GitHub/Views/GitHubTokenDialogView.axaml
GenHub/GenHub/Features/GitHub/Views/GitHubTokenDialogView.axaml.cs
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/Services/VelopackUpdateManagerTests.cs
GenHub/GenHub.Core/Helpers/FileSizeFormatter.cs

🧰 Additional context used

🧬 Code graph analysis (3)

GenHub/GenHub/Features/Settings/ViewModels/SettingsViewModel.cs (3)

GenHub/GenHub.Core/Constants/AppConstants.cs (1)

AppConstants (10-137)

GenHub/GenHub/Infrastructure/Extensions/LoggerExtensions.cs (1)

LogError (46-51)

GenHub/GenHub/Common/Services/UserSettingsService.cs (1)

Update (79-102)

GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/GameProfileModuleTests.cs (2)

GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/SharedViewModelModuleTests.cs (1)

IConfigurationProviderService (141-158)

GenHub/GenHub/Features/GameProfiles/ViewModels/GameSettingsViewModel.cs (1)

Core (183-203)

GenHub/GenHub/Features/AppUpdate/Services/VelopackUpdateManager.cs (2)

GenHub/GenHub.Core/Constants/AppConstants.cs (1)

AppConstants (10-137)

GenHub/GenHub/Infrastructure/Extensions/LoggerExtensions.cs (1)

LogError (46-51)

🪛 actionlint (1.7.9)

.github/workflows/ci.yml

89-89: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

247-247: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

🪛 GitHub Check: Build Linux

GenHub/GenHub.Core/Models/AppUpdate/PullRequestInfo.cs

[warning] 31-31:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 26-26:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 21-21:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 16-16:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 11-11:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 31-31:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 26-26:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 21-21:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 16-16:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 11-11:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

🪛 GitHub Check: Build Windows

GenHub/GenHub/Common/ViewModels/MainViewModel.cs

[warning] 382-382:
Single-line comment should be preceded by blank line (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1515.md)

[warning] 381-381:
Closing brace should be followed by blank line (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1513.md)

GenHub/GenHub.Core/Models/AppUpdate/PullRequestInfo.cs

[warning] 31-31:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 26-26:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 21-21:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 16-16:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 11-11:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 31-31:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 26-26:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 21-21:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 16-16:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 11-11:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 31-31:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 26-26:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 21-21:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 16-16:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

[warning] 11-11:
The 'public' modifier should appear before 'required' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1206.md)

🔇 Additional comments (23)

GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/GameProfileModuleTests.cs (2)

7-7: LGTM! Formatting improvements enhance readability.

The empty lines appropriately separate logical sections and the trailing newline follows best practices.

Also applies to: 170-170, 305-305

141-141: LGTM! Consistent dependency setup.

The registration of IConfigurationProviderService aligns with the dependency setup pattern used in other tests throughout the file.

GenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameProfiles/GameProcessManagerTests.cs (1)

220-220: Past review concern about missing positive-path test remains valid.

The concern raised in the previous review regarding the removal of the explicit StartProcessAsync_WithValidExecutablePath_ShouldReturnSuccess() test is still applicable. While the success path is indirectly tested at lines 137 and 200, restoring a dedicated positive-path test would improve test clarity and maintainability as previously noted.

GenHub/GenHub/Common/Views/MainView.axaml (2)

191-204: LGTM - Layout reorganization looks good.

The update notification button placement on the left with branding on the right provides a cleaner visual hierarchy. The button's visibility binding and command remain correctly wired.

213-218: Minor formatting adjustment - no functional change.

The version TextBlock reformatting maintains the same binding and styling, just with improved line breaks for readability.

GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/SharedViewModelModuleTests.cs (2)

99-105: LGTM - Mock registrations are clean and properly scoped.

Using fully-qualified type names (GenHub.Core.Interfaces.Shortcuts.IShortcutService and GenHub.Core.Interfaces.GitHub.IGitHubTokenStorage) avoids ambiguity and the previous duplicate registration issue has been resolved.

112-112: Registration order adjusted appropriately.

Moving AddCasServices() before AddContentPipelineServices() ensures proper dependency resolution order.

GenHub/GenHub/Features/GameProfiles/Services/ProfileContentLoader.cs (1)

495-522: Good improvement: Named tuple return type improves readability.

The change from an unnamed tuple to (GameInstallation? Installation, GameClient? Client) makes the call sites clearer and more self-documenting.

GenHub/Directory.Build.props (2)

14-26: Well-structured CI metadata integration.

The approach of defining defaults here with CI overrides via -p: flags is clean. The conditional InformationalVersion enrichment (e.g., 0.0.150+abc1234) follows semantic versioning conventions for build metadata.

29-43: Good use of AssemblyMetadataAttribute for runtime access.

Embedding GitShortHash, PullRequestNumber, and BuildChannel as assembly metadata enables runtime access via reflection (as used by BuildInfoConstants per the AI summary). This is the idiomatic approach for exposing build-time values.

GenHub/GenHub.Core/Models/Enums/UpdateChannel.cs (1)

1-22: LGTM!

The enum is well-structured with clear XML documentation explaining each update channel's purpose. The naming is intuitive and follows .NET conventions.

GenHub/GenHub.Core/Constants/GitHubConstants.cs (1)

280-282: LGTM!

The constant provides a convenient URL for PAT creation with the necessary repo scope pre-configured, which aligns with the artifact download requirements.

GenHub/GenHub/Common/ViewModels/MainViewModel.cs (1)

355-392: Version comparison logic looks correct.

The approach of stripping build metadata before comparison and checking against dismissed versions is sound. The null-safety checks are appropriate.

GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationWindow.axaml (1)

9-9: Icon and color scheme updates look good.

Window icon is properly set and the background gradient colors are consistent with a dark theme.

Also applies to: 32-33

GenHub/GenHub/Features/AppUpdate/Services/SimpleHttpServer.cs (1)

86-92: Consider disposing _cts only once.

If Stop() is called before Dispose(), and you add the suggested fix to dispose _cts in Stop(), ensure Dispose() handles the already-disposed case gracefully. The current null-conditional _cts?.Dispose() handles this, but be mindful when applying the Stop() fix.

GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationView.axaml (1)

7-7: LGTM - Clean layout structure with proper scrolling.

The refactored layout uses a sensible Grid with RowDefinitions="Auto,*,Auto" pattern: fixed header (Row 0), scrollable content (Row 1), and fixed action buttons (Row 2). The ScrollViewer in Row 1 properly wraps the dynamic content sections, and the visibility bindings for different states (update available, error, PR merged warning, Open PRs) are well-organized within the StackPanel.

Also applies to: 38-290

GenHub/GenHub/Features/Settings/Views/SettingsView.axaml (1)

452-483: Well-structured Updates section with clear channel guidance.

The new Updates section provides good UX with clear labels for update channels (Stable/Prerelease/Artifacts), appropriate gating of PAT-related features behind HasGitHubPat, and a convenient "Manage Updates & PRs" button for detailed update management.

GenHub/GenHub/Features/AppUpdate/Interfaces/IVelopackUpdateManager.cs (1)

14-110: Well-designed interface with comprehensive update management surface.

The interface cleanly separates:

GitHub Releases flow (CheckForUpdatesAsync, DownloadUpdatesAsync, ApplyUpdatesAndRestart/Exit)

Artifact/CI build flow (CheckForArtifactUpdatesAsync, GetOpenPullRequestsAsync, InstallPrArtifactAsync)

State properties for UI binding (CurrentChannel, HasArtifactUpdateAvailable, LatestArtifactUpdate, SubscribedPrNumber, IsPrMergedOrClosed)

Good use of CancellationToken and IProgress<UpdateProgress> for async operations.

GenHub/GenHub.Core/Constants/AppConstants.cs (1)

17-94: Well-structured build metadata integration.

The lazy initialization for assembly metadata is thread-safe, and the FullDisplayVersion logic cleanly handles local, CI, and PR build variants. The GetAssemblyMetadata helper is appropriately defensive with null coalescing.

GenHub/GenHub/Features/AppUpdate/ViewModels/UpdateNotificationViewModel.cs (1)

260-299: PR update check logic is well-structured.

The prioritization of subscribed PR builds over main branch releases is clear. Version comparison correctly strips build metadata (Split('+')[0]) to avoid false positives from hash differences. The early-return pattern keeps the flow readable.

GenHub/GenHub/Features/AppUpdate/Services/VelopackUpdateManager.cs (3)

97-99: Per-run state reset is correct.

Resetting _hasUpdateFromGitHub and _latestVersionFromGitHub at the start of each check prevents stale UI state from persisting across consecutive update checks.

741-750: Direct VelopackAsset construction for PR artifacts is a good approach.

Bypassing Velopack's version check by constructing the asset directly allows users to install any PR build regardless of version comparison. This is appropriate for the PR artifact use case where switching between builds is intentional.

918-928: SecureStringToString correctly zeroes allocated memory.

Using Marshal.ZeroFreeGlobalAllocUnicode in the finally block ensures the plaintext is cleared from unmanaged memory, which is the correct pattern for handling SecureString conversion.

coderabbitai

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationView.axaml (1)
1-5: Add xmlns for ObjectConverters to resolve the converter reference.

Line 246 uses {x:Static ObjectConverters.IsNotNull} but ObjectConverters is in Avalonia.Data.Converters, not the default namespace. This will fail at runtime.
 <UserControl xmlns="https://github.com/avaloniaui"
     xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
     xmlns:vm="using:GenHub.Features.AppUpdate.ViewModels"
+    xmlns:conv="using:Avalonia.Data.Converters"
     x:Class="GenHub.Features.AppUpdate.Views.UpdateNotificationView"
     x:DataType="vm:UpdateNotificationViewModel">
And update line 246:
-    <Border ... IsVisible="{Binding SubscribedPr, Converter={x:Static ObjectConverters.IsNotNull}}">
+    <Border ... IsVisible="{Binding SubscribedPr, Converter={x:Static conv:ObjectConverters.IsNotNull}}">
Also applies to: 246-246
GenHub/GenHub/Features/AppUpdate/ViewModels/UpdateNotificationViewModel.cs (1)
82-85: CanInstallPrArtifact won't update when IsInstalling changes.

The _isInstalling property only has [NotifyCanExecuteChangedFor(nameof(InstallUpdateCommand))] but CanInstallPrArtifact (line 537) also depends on IsInstalling. Add the attribute to update both commands:
     [ObservableProperty]
     [NotifyPropertyChangedFor(nameof(InstallButtonText))]
     [NotifyCanExecuteChangedFor(nameof(InstallUpdateCommand))]
+    [NotifyCanExecuteChangedFor(nameof(InstallPrArtifactCommand))]
     private bool _isInstalling;

♻️ Duplicate comments (26)

GenHub/GenHub/Features/GitHub/ViewModels/GitHubTokenDialogViewModel.cs (6)

52-67: Race condition: _secureToken is not thread-safe.

If SetToken is called while ValidateTokenAsync or SaveAsync is executing, _secureToken could be disposed mid-operation, causing ObjectDisposedException or reading stale/disposed data.

Past review suggested adding synchronization with SemaphoreSlim to serialize access across SetToken, ValidateTokenAsync, SaveAsync, and GetPlainToken.

117-121: Do not dispose HttpClient created from IHttpClientFactory.

Line 117 uses using var client = httpClientFactory.CreateClient();, which disposes the client. HttpClients created from IHttpClientFactory should NOT be disposed, as the factory manages their lifecycle and connection pooling. Disposing them can lead to socket exhaustion and performance issues.

Note: A past review marked this as addressed, but the code still contains the using var disposal pattern.

126-143: Scope check may produce false positives with substring matching.

Line 129 uses Contains("repo", StringComparison.OrdinalIgnoreCase), which could match unintended scope names like "repository" or "repos".

Past review suggested splitting the scope string and checking for exact matches to ensure only the "repo" scope is matched, not substrings.

166-172: Consider sanitizing exception messages displayed to users.

Line 168 exposes ex.Message directly in the UI. While not immediately critical, exception messages can sometimes contain sensitive details or internal paths.

Past review suggested using a generic error message or logging the full exception while showing a user-friendly message.

206-212: Sanitize exception message displayed to users.

Similar to line 168, line 208 exposes ex.Message directly in the UI, which could leak sensitive information.

Past review suggested displaying a generic message while keeping the full exception logged (line 211) for debugging purposes.

224-238: Add platform guards to GetPlainToken or move to platform-specific implementations.

The GetPlainToken method uses Marshal.SecureStringToGlobalAllocUnicode, a Windows-only Win32 API. Since the application targets both Windows and Linux, this code will fail at runtime on non-Windows platforms.

Past review suggested adding an OperatingSystem.IsWindows() guard or throwing a PlatformNotSupportedException, or relocating this method to a Windows-specific implementation similar to WindowsGitHubTokenStorage.
.github/workflows/ci.yml (3)
88-95: Update actions/cache from v3 to v4.

Both NuGet cache steps still use actions/cache@v3. Actionlint confirms v3 runners are no longer supported on GitHub Actions. The workflow already uses v4 for other actions (checkout, setup-dotnet, upload-artifact).

Apply this diff to both cache steps:
       - name: Cache NuGet Packages
-        uses: actions/cache@v3
+        uses: actions/cache@v4
         with:
           path: ~/.nuget/packages
Also applies to: 246-253

383-390: Parameterize the release tag to use computed build version.

The release tag uses v0.0.${{ github.run_number }} which duplicates version logic. If versioning changes, this will diverge from actual build versions. Consider exposing VERSION as a job output from build-windows and referencing it here.

Add outputs to build-windows job and reference them:
build-windows:
  outputs:
    version: ${{ steps.buildinfo.outputs.VERSION }}
-          tag_name: v0.0.${{ github.run_number }}
-          name: GenHub Alpha ${{ github.run_number }}
+          tag_name: v${{ needs.build-windows.outputs.version }}
+          name: GenHub ${{ needs.build-windows.outputs.version }}
278-284: Critical: Linux build is missing CHANNEL export.

The Linux Extract Build Info step computes CHANNEL (lines 265/268) but does not export it to $GITHUB_OUTPUT. Line 284 references ${{ steps.buildinfo.outputs.CHANNEL }} which will be empty, causing BuildChannel to be blank in Linux assemblies.

Add the missing export after line 280:
           echo "SHORT_HASH=$SHORT_HASH" >> $GITHUB_OUTPUT
           echo "PR_NUMBER=$PR_NUMBER" >> $GITHUB_OUTPUT
           echo "VERSION=$VERSION" >> $GITHUB_OUTPUT
+          echo "CHANNEL=$CHANNEL" >> $GITHUB_OUTPUT
GenHub/GenHub/Features/AppUpdate/Services/SimpleHttpServer.cs (4)
46-58: Validate port before constructing the listener.

The constructor doesn't validate the port range, which can lead to confusing HttpListenerException for invalid values.
     public SimpleHttpServer(string nupkgPath, string releasesPath, int port, ILogger logger)
     {
+        if (port is < 1 or > 65535)
+            throw new ArgumentOutOfRangeException(nameof(port), "Port must be 1..65535.");
         _nupkgPath = nupkgPath ?? throw new ArgumentNullException(nameof(nupkgPath));
80-100: Reset _started flag in Stop() to allow server restart.

Start() uses Interlocked.CompareExchange to set _started to 1, but Stop() never resets it to 0. After calling Stop(), subsequent Start() calls will be no-ops.
         _logger.LogInformation("HTTP server stopped");
+
+        _serverTask = null;
+        _cts?.Dispose();
+        _cts = null;
+        Interlocked.Exchange(ref _started, 0);
     }
147-205: Ensure HTTP response is always closed using finally.

If an exception occurs before response.Close() (line 199), the response remains open, which can cause clients to hang and exhaust listener resources.
     private async Task ProcessRequestAsync(HttpListenerContext context)
     {
+        var response = context.Response;
         try
         {
             var request = context.Request;
-            var response = context.Response;
             // ... handling logic ...
-            response.Close();
         }
         catch (Exception ex)
         {
             _logger.LogError(ex, "Error processing HTTP request");
         }
+        finally
+        {
+            try { response.Close(); } catch { /* best-effort */ }
+        }
     }
129-129: Add exception handling for fire-and-forget task.

The _ = Task.Run(...) discards the task, so exceptions in ProcessRequestAsync will be unobserved. Consider adding a continuation to log failures.
-                _ = Task.Run(() => ProcessRequestAsync(context), cancellationToken);
+                var requestTask = Task.Run(() => ProcessRequestAsync(context), cancellationToken);
+                _ = requestTask.ContinueWith(t =>
+                {
+                    if (t.IsFaulted && t.Exception != null)
+                        _logger.LogError(t.Exception, "Unhandled error in request processing");
+                }, TaskScheduler.Default);
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/Services/VelopackUpdateManagerTests.cs (1)

164-166: Log assertion still uses permissive OR logic.

The assertion accepts either "Velopack" OR "Update", which was flagged in a previous review as overly permissive. The recommendation was to use AND logic to match both "Velopack" and "UpdateManager".
GenHub/GenHub.Core/Models/AppUpdate/PullRequestInfo.cs (1)
53-56: Use null-safe comparison for IsOpen.

State is marked required but can still be null at runtime (e.g., from deserialization). Calling State.Equals(...) will throw if null.
-    public bool IsOpen => State.Equals("open", StringComparison.OrdinalIgnoreCase);
+    public bool IsOpen => string.Equals(State, "open", StringComparison.OrdinalIgnoreCase);
GenHub/GenHub.Core/Models/AppUpdate/ArtifactUpdateInfo.cs (2)
72-85: Guard against empty GitHash in DisplayVersion.

If GitHash is empty or whitespace, the formatted string will produce an odd UI display like "v1.0.0 ()". Consider conditionally omitting the hash suffix.
 public string DisplayVersion
 {
     get
     {
+        var hashPart = string.IsNullOrWhiteSpace(GitHash) ? string.Empty : $" ({GitHash})";
         if (PullRequestNumber.HasValue)
         {
-            // Strip any build metadata from version (everything after +)
             var baseVersion = Version.Split('+')[0];
-            return $"v{baseVersion} ({GitHash})";
+            return $"v{baseVersion}{hashPart}";
         }

-        return $"v{Version} ({GitHash})";
+        return $"v{Version}{hashPart}";
     }
 }
59-62: Consider DateTimeOffset for CreatedAt.

GitHub API timestamps are UTC. DateTimeOffset preserves offset information and avoids DateTime.Kind ambiguity. This is a minor improvement for correctness.
GenHub/GenHub/Features/Settings/ViewModels/SettingsViewModel.cs (3)

698-736: LoadPatStatusAsync treats "token exists" as "token valid" without actual validation.

Setting IsPatValid = true based solely on HasToken() (line 713) means expired or revoked PATs will show as valid. Consider validating the token with an API call before marking it valid.

This was flagged in a previous review.

768-793: PAT validation logic always succeeds after saving.

The condition at line 778 if (artifact != null || _gitHubTokenStorage.HasToken()) will always be true because the token was just saved at line 768. This means invalid PATs (e.g., with wrong scopes) will incorrectly show as validated.

This issue was raised in a previous review. The || _gitHubTokenStorage.HasToken() clause should be removed and a proper fallback message should be shown when no artifacts are found.

875-884: Marshal AvailableArtifacts mutations to UI thread.

AvailableArtifacts.Clear() (line 876) and AvailableArtifacts.Add() (line 883) can execute off the UI thread after the await, which can crash Avalonia-bound collections.

This was flagged in a previous review. Wrap collection mutations in Dispatcher.UIThread.InvokeAsync().

GenHub/GenHub/Features/AppUpdate/ViewModels/UpdateNotificationViewModel.cs (4)

148-166: InitializeAsync lacks top-level exception handling.

The method calls LoadPullRequestsAsync and CheckForUpdatesAsync without a surrounding try-catch. Combined with the fire-and-forget pattern at line 142, exceptions will be unobserved.

This was flagged in a previous review. Wrap the method body in try-catch and set HasError/ErrorMessage on failure.

610-611: AvailablePullRequests.Clear() should be on UI thread.

Line 611 calls Clear() directly, but this can execute off the UI thread after awaits in the calling context, potentially crashing the Avalonia-bound collection.

This was flagged in a previous review. Wrap in Dispatcher.UIThread.InvokeAsync().

666-667: Fire-and-forget SaveAsync in SubscribeToPr discards errors.

Line 667 uses _ = _userSettingsService.SaveAsync() without error handling. If saving fails, the user won't know their subscription wasn't persisted.

This was flagged in a previous review. Add a ContinueWith to log failures.

694-695: Fire-and-forget SaveAsync in UnsubscribeFromPr also discards errors.

Same pattern as SubscribeToPr - consider extracting a helper method for save operations with error logging.

This was flagged in a previous review.

GenHub/GenHub/Features/AppUpdate/Services/VelopackUpdateManager.cs (2)

67-84: Debug logging in property getters will spam logs.

HasUpdateAvailableFromGitHub and LatestVersionFromGitHub getters log on every access. These properties are likely polled frequently by UI bindings, which will flood logs and add overhead.

This was flagged in a previous review. Remove the logger.LogDebug calls from property getters.

388-493: N+1 GitHub API pattern still present - fetches workflow runs inside PR loop.

For each open PR, the code fetches workflow runs (lines 391-392) and then artifacts. With 30 PRs and 5 runs per page, this makes 30+ API calls, quickly exhausting rate limits.

This was flagged in a previous review but appears to still be present. Fetch workflow runs once before the PR loop, group by PR number, then look up runs for each PR.

🧹 Nitpick comments (9)

GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/SharedViewModelModuleTests.cs (1)
99-101: Consider using a type alias for brevity.

The fully-qualified type name GenHub.Core.Interfaces.Shortcuts.IShortcutService is verbose. Consider adding a using alias at the top of the file to improve readability.

Add this using statement near the top of the file (after line 13):
+using IShortcutService = GenHub.Core.Interfaces.Shortcuts.IShortcutService;
Then simplify the mock registration:
-        // Mock IShortcutService to avoid dependency issues
-        var shortcutServiceMock = new Mock<GenHub.Core.Interfaces.Shortcuts.IShortcutService>();
-        services.AddSingleton<GenHub.Core.Interfaces.Shortcuts.IShortcutService>(shortcutServiceMock.Object);
+        // Mock IShortcutService to avoid dependency issues
+        var shortcutServiceMock = new Mock<IShortcutService>();
+        services.AddSingleton<IShortcutService>(shortcutServiceMock.Object);
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/ViewModels/UpdateNotificationViewModelTests.cs (1)
25-26: Consider extracting mock setup to a helper method.

The IUserSettingsService mock setup is duplicated across all three tests. Given the existing pattern in SharedViewModelModuleTests.cs with CreateMockUserSettingsService(), consider extracting this to a reusable helper for consistency and maintainability.
+    private static Mock<IUserSettingsService> CreateMockUserSettingsService()
+    {
+        var mock = new Mock<IUserSettingsService>();
+        mock.Setup(x => x.Get()).Returns(new GenHub.Core.Models.Common.UserSettings());
+        return mock;
+    }
+
     [Fact]
     public async Task CheckForUpdatesCommand_WhenNoUpdateAvailable_UpdatesStatus()
     {
         var mockVelopack = new Mock<IVelopackUpdateManager>();
         mockVelopack.Setup(x => x.CheckForUpdatesAsync(It.IsAny<CancellationToken>()))
            .ReturnsAsync((Velopack.UpdateInfo?)null);

-        var mockUserSettings = new Mock<IUserSettingsService>();
-        mockUserSettings.Setup(x => x.Get()).Returns(new GenHub.Core.Models.Common.UserSettings());
+        var mockUserSettings = CreateMockUserSettingsService();
Also applies to: 45-46, 65-66
.github/workflows/ci.yml (1)
373-381: Consider logging when optional files are missing.

The || true patterns silently ignore missing files. For debugging release issues, consider adding explicit checks or logging when optional files (Linux nupkg, RELEASES, Setup.exe) are absent.
-          cp release-assets/linux/*.nupkg final-assets/ || true
+          cp release-assets/linux/*.nupkg final-assets/ 2>/dev/null || echo "Note: Linux nupkg not found (optional)"
docs/velopack-integration.md (1)

111-128: Consider documenting the Prerelease channel.

The documentation describes Stable and Artifacts channels, but the UpdateChannel enum also includes Prerelease. Since UserSettings defaults to UpdateChannel.Prerelease, it would be helpful to explain what this channel provides (e.g., GitHub Releases marked as prerelease).
GenHub/GenHub/Features/Settings/ViewModels/SettingsViewModel.cs (1)
927-931: Fire-and-forget LoadArtifactsAsync may swallow exceptions.

Line 930 discards the task from LoadArtifactsAsync(). Consider adding a continuation to log failures, similar to the pattern used for SaveAsync above.
         if (value == UpdateChannel.Artifacts && HasGitHubPat)
         {
-            _ = LoadArtifactsAsync();
+            _ = LoadArtifactsAsync().ContinueWith(t =>
+            {
+                if (t.IsFaulted)
+                    _logger.LogError(t.Exception, "Failed to load artifacts after channel change");
+            }, TaskScheduler.Default);
         }
GenHub/GenHub/Features/AppUpdate/Services/VelopackUpdateManager.cs (4)
208-215: Dispose SecureString token after use.

The token from LoadTokenAsync() is not wrapped in using, leaving sensitive data in memory longer than necessary.
-            var token = await gitHubTokenStorage.LoadTokenAsync();
+            using var token = await gitHubTokenStorage.LoadTokenAsync();
             if (token == null)
             {
                 logger.LogWarning("Failed to load GitHub PAT");
                 return null;
             }
334-341: Same SecureString disposal issue in GetOpenPullRequestsAsync.

Apply the same using pattern here.
-            var token = await gitHubTokenStorage.LoadTokenAsync();
+            using var token = await gitHubTokenStorage.LoadTokenAsync();
668-674: SecureString token not disposed in InstallPrArtifactAsync.

Same issue as other methods - wrap the token in using.
-            var token = await gitHubTokenStorage.LoadTokenAsync();
+            using var token = await gitHubTokenStorage.LoadTokenAsync();
922-929: Minor TOCTOU race in FindAvailablePort.

The port is released after listener.Stop(), creating a window where another process could claim it. This is an edge case but worth noting.

Consider implementing retry logic if HTTP server binding fails, or document this limitation.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a28a068 and 37cacfd.

📒 Files selected for processing (30)

.github/workflows/ci.yml (5 hunks)
GenHub/Directory.Build.props (1 hunks)
GenHub/GenHub.Core/Constants/AppConstants.cs (5 hunks)
GenHub/GenHub.Core/Constants/BuildInfoConstants.cs (1 hunks)
GenHub/GenHub.Core/Constants/GitHubConstants.cs (1 hunks)
GenHub/GenHub.Core/Models/AppUpdate/ArtifactUpdateInfo.cs (1 hunks)
GenHub/GenHub.Core/Models/AppUpdate/PullRequestInfo.cs (1 hunks)
GenHub/GenHub.Core/Models/Common/UserSettings.cs (2 hunks)
GenHub/GenHub.Core/Models/Enums/UpdateChannel.cs (1 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Constants/AppConstantsTests.cs (1 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/Services/VelopackUpdateManagerTests.cs (12 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/ViewModels/UpdateNotificationViewModelTests.cs (4 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameProfiles/GameProcessManagerTests.cs (0 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/GameProfileModuleTests.cs (3 hunks)
GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/SharedViewModelModuleTests.cs (2 hunks)
GenHub/GenHub/Common/ViewModels/MainViewModel.cs (1 hunks)
GenHub/GenHub/Common/Views/MainView.axaml (2 hunks)
GenHub/GenHub/Features/AppUpdate/Interfaces/IVelopackUpdateManager.cs (3 hunks)
GenHub/GenHub/Features/AppUpdate/Services/SimpleHttpServer.cs (1 hunks)
GenHub/GenHub/Features/AppUpdate/Services/VelopackUpdateManager.cs (7 hunks)
GenHub/GenHub/Features/AppUpdate/ViewModels/UpdateNotificationViewModel.cs (9 hunks)
GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationView.axaml (5 hunks)
GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationWindow.axaml (3 hunks)
GenHub/GenHub/Features/GitHub/ViewModels/GitHubTokenDialogViewModel.cs (1 hunks)
GenHub/GenHub/Features/GitHub/Views/GitHubTokenDialogView.axaml (1 hunks)
GenHub/GenHub/Features/GitHub/Views/GitHubTokenDialogView.axaml.cs (1 hunks)
GenHub/GenHub/Features/Settings/ViewModels/SettingsViewModel.cs (8 hunks)
GenHub/GenHub/Features/Settings/Views/SettingsView.axaml (2 hunks)
GenHub/GenHub/Features/Settings/Views/SettingsView.axaml.cs (1 hunks)
docs/velopack-integration.md (4 hunks)

💤 Files with no reviewable changes (1)

GenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameProfiles/GameProcessManagerTests.cs

🚧 Files skipped from review as they are similar to previous changes (11)

GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationWindow.axaml
GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/GameProfileModuleTests.cs
GenHub/GenHub/Common/Views/MainView.axaml
GenHub/GenHub/Features/Settings/Views/SettingsView.axaml.cs
GenHub/GenHub.Tests/GenHub.Tests.Core/Constants/AppConstantsTests.cs
GenHub/GenHub/Features/Settings/Views/SettingsView.axaml
GenHub/GenHub/Features/GitHub/Views/GitHubTokenDialogView.axaml
GenHub/GenHub.Core/Models/Common/UserSettings.cs
GenHub/GenHub.Core/Constants/BuildInfoConstants.cs
GenHub/GenHub.Core/Constants/GitHubConstants.cs
GenHub/GenHub/Features/GitHub/Views/GitHubTokenDialogView.axaml.cs

🧰 Additional context used

🧬 Code graph analysis (4)

GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/ViewModels/UpdateNotificationViewModelTests.cs (1)

GenHub/GenHub/Features/AppUpdate/ViewModels/UpdateNotificationViewModel.cs (2)

UpdateNotificationViewModel (24-699)

UpdateNotificationViewModel (122-143)

GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/Services/VelopackUpdateManagerTests.cs (2)

GenHub/GenHub.Core/Models/Common/UserSettings.cs (1)

UserSettings (7-148)

GenHub/GenHub/Features/AppUpdate/Interfaces/IVelopackUpdateManager.cs (5)

Task (21-21)

Task (29-29)

Task (37-37)

Task (46-46)

Task (109-109)

GenHub/GenHub/Common/ViewModels/MainViewModel.cs (1)

GenHub/GenHub.Core/Constants/AppConstants.cs (1)

AppConstants (9-136)

GenHub/GenHub/Features/AppUpdate/Services/VelopackUpdateManager.cs (5)

GenHub/GenHub.Core/Constants/AppConstants.cs (1)

AppConstants (9-136)

GenHub/GenHub/Infrastructure/Extensions/LoggerExtensions.cs (1)

LogError (46-51)

GenHub/GenHub/Features/AppUpdate/Interfaces/IVelopackUpdateManager.cs (5)

Task (21-21)

Task (29-29)

Task (37-37)

Task (46-46)

Task (109-109)

GenHub/GenHub/Features/AppUpdate/Services/SimpleHttpServer.cs (7)

Task (110-145)

Task (147-205)

SimpleHttpServer (13-206)

SimpleHttpServer (46-58)

Start (63-75)

Dispose (103-108)

Stop (80-100)

GenHub/GenHub/Features/AppUpdate/ViewModels/UpdateNotificationViewModel.cs (3)

Task (148-166)

Task (242-369)

Dispose (231-236)

🪛 actionlint (1.7.9)

.github/workflows/ci.yml

89-89: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

247-247: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

🪛 markdownlint-cli2 (0.18.1)

docs/velopack-integration.md

74-74: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

338-338: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🔇 Additional comments (26)

GenHub/GenHub.Tests/GenHub.Tests.Core/Infrastructure/DependencyInjection/SharedViewModelModuleTests.cs (2)
103-105: Mock registration looks good.

The IGitHubTokenStorage mock is correctly set up to support the new Velopack update functionality. The registration pattern is appropriate for test isolation.

Optional: Similar to the IShortcutService mock above, consider using a type alias to reduce verbosity:
+using IGitHubTokenStorage = GenHub.Core.Interfaces.GitHub.IGitHubTokenStorage;
Then simplify:
-        // Mock IGitHubTokenStorage to avoid dependency issues
-        var tokenStorageMock = new Mock<GenHub.Core.Interfaces.GitHub.IGitHubTokenStorage>();
-        services.AddSingleton<GenHub.Core.Interfaces.GitHub.IGitHubTokenStorage>(tokenStorageMock.Object);
+        // Mock IGitHubTokenStorage to avoid dependency issues
+        var tokenStorageMock = new Mock<IGitHubTokenStorage>();
+        services.AddSingleton<IGitHubTokenStorage>(tokenStorageMock.Object);
112-112: Service registration order looks correct.

The AddCasServices() call is appropriately positioned before AddContentPipelineServices(), ensuring CAS dependencies are available when the content pipeline is initialized.
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/ViewModels/UpdateNotificationViewModelTests.cs (1)

1-75: LGTM overall.

The tests correctly accommodate the new IUserSettingsService dependency and verify the expected behavior. The test structure is clean and assertions are appropriate.

docs/velopack-integration.md (1)

1-482: Well-structured and comprehensive documentation.

The Velopack integration guide provides thorough coverage of the update system, including versioning, channels, troubleshooting, and security considerations. The examples are clear and match the implementation.

GenHub/GenHub.Core/Models/Enums/UpdateChannel.cs (1)

1-22: LGTM!

The enum is well-documented and provides clear semantics for each update channel. The three values appropriately cover stable releases, prerelease versions, and CI artifacts.

GenHub/GenHub/Common/ViewModels/MainViewModel.cs (1)

355-393: Well-structured version comparison logic.

The update check properly:

Strips build metadata (+...) for accurate version comparison

Checks against user-dismissed versions to avoid nagging

Compares against current version to hide redundant notifications

The control flow correctly ensures HasUpdateAvailable is only true when neither the dismissed nor current version matches the latest base version.

GenHub/GenHub.Tests/GenHub.Tests.Core/Features/AppUpdate/Services/VelopackUpdateManagerTests.cs (2)

172-189: Good test coverage for PAT-less artifact check.

The test correctly verifies that CheckForArtifactUpdatesAsync returns null and HasArtifactUpdateAvailable is false when no PAT is configured. This covers an important edge case for users without GitHub tokens.

191-195: Clean helper extraction for manager instantiation.

The CreateManager() helper centralizes dependency injection, making tests maintainable when the VelopackUpdateManager constructor evolves.

GenHub/GenHub.Core/Models/AppUpdate/PullRequestInfo.cs (1)

1-52: Well-structured PR info record.

Good use of required for essential properties, nullable for optional ones, and computed properties for derived state. The XML documentation is thorough.

GenHub/GenHub.Core/Models/AppUpdate/ArtifactUpdateInfo.cs (1)

14-57: Record structure is well-designed.

Clear separation of constructor parameters and re-exposed properties with proper documentation. The IsPrBuild computed property provides a clean abstraction.

GenHub/GenHub.Core/Constants/AppConstants.cs (3)

26-33: Good use of lazy initialization for metadata access.

Lazy<T> ensures thread-safe, one-time initialization of assembly metadata. The default values (empty string for optional fields, "Dev" for BuildChannel) are sensible fallbacks.

75-93: Clean conditional formatting logic.

FullDisplayVersion handles all build scenarios correctly:

Local builds: just the version

CI builds: version with git hash

PR builds: version with PR number and git hash

The early return pattern makes the logic easy to follow.

124-135: Well-encapsulated metadata access.

The GetAssemblyMetadata helper properly encapsulates reflection-based metadata lookup with null-safe handling.

GenHub/Directory.Build.props (3)

3-9: Clear versioning baseline for alpha phase.

The 0.0.X format with documented CI override pattern (0.0.{runNumber} or 0.0.{runNumber}-pr{prNumber}) provides a predictable versioning scheme during early development.

14-26: Well-designed CI metadata injection.

The empty defaults allow local builds to work seamlessly while CI can override via -p:GitShortHash=... -p:PullRequestNumber=.... The conditional InformationalVersion only appends the hash when available.

29-43: Correct pattern for embedding runtime metadata.

The AssemblyMetadataAttribute items will be compiled into the assembly, allowing AppConstants.GetAssemblyMetadata() to retrieve them at runtime. This enables build context awareness without code generation.

GenHub/GenHub/Features/AppUpdate/Views/UpdateNotificationView.axaml (2)

38-114: LGTM - Update available section is well-structured.

The update available UI section with version display, release notes button, and installation progress indicator is cleanly organized with appropriate bindings.

170-243: LGTM - Open PRs section with proper item template.

The PR list uses appropriate ItemsControl with DataTemplate, and the Subscribe button correctly navigates the visual tree to find the parent ViewModel's command.

GenHub/GenHub/Features/Settings/ViewModels/SettingsViewModel.cs (1)

848-861: LGTM - OpenUpdateWindow is a clean fire-and-forget UI action.

The method properly wraps window creation in try-catch and logs failures.

GenHub/GenHub/Features/AppUpdate/Interfaces/IVelopackUpdateManager.cs (2)

23-37: LGTM - Well-documented artifact and PR API surface.

The new methods for artifact updates and PR discovery are cleanly designed with appropriate return types and XML documentation.

76-110: LGTM - Properties and methods for PR subscription workflow.

The interface properly exposes subscription state (SubscribedPrNumber, IsPrMergedOrClosed) and the installation method with progress support.

GenHub/GenHub/Features/AppUpdate/ViewModels/UpdateNotificationViewModel.cs (2)

259-298: LGTM - PR subscription takes priority in update check flow.

The logic correctly prioritizes subscribed PR artifacts over main branch releases, with proper version comparison stripping build metadata.

484-532: LGTM - PR artifact installation with proper progress reporting.

The InstallPrArtifactAsync method has appropriate error handling, progress reporting, and cleanup in the finally block.

GenHub/GenHub/Features/AppUpdate/Services/VelopackUpdateManager.cs (3)

648-885: LGTM - Comprehensive PR artifact installation flow.

The InstallPrArtifactAsync method handles the full workflow: downloading artifacts, extracting nupkg, creating releases JSON, hosting a local server, and applying updates via Velopack. Error handling and cleanup in finally blocks are appropriate.

906-920: LGTM - Hash calculation helpers are straightforward and correct.

CalculateSHA1 and CalculateSHA256 properly dispose streams and cryptographic objects.

836-855: No race condition exists. DownloadUpdatesAsync is called before ApplyUpdatesAndRestart, so the update is already downloaded to local disk when ApplyUpdatesAndRestart is invoked. ApplyUpdatesAndRestart applies updates and optionally relaunches the app—it does not download from the server. The HTTP server disposal in the finally block occurs after the download is complete.

Likely an incorrect or invalid review comment.

coderabbitai · 2025-12-17T13:14:40Z

docs/velopack-integration.md

+```
+0.0.{RUN_NUMBER}[-pr{PR_NUMBER}][+{BUILD_METADATA}]
+```


⚠️ Potential issue | 🟡 Minor

Add language specifiers to fenced code blocks.

Markdownlint flags that these code blocks should have a language specified. Use text or plaintext for non-code format examples.

-``` +```text 0.0.{RUN_NUMBER}[-pr{PR_NUMBER}][+{BUILD_METADATA}]

```diff -``` +```text Display: v0.0.160-pr3 (051baf8) Full version: 0.0.160-pr3+051baf8.051baf832a894f542e60d3e4c2471c7d4dd753da Comparison: 0.0.160-pr3

Also applies to: 338-342 <details> <summary>🧰 Tools</summary> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> 74-74: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> <details> <summary>🤖 Prompt for AI Agents</summary>

In docs/velopack-integration.md around lines 74 to 76 (and also lines 338 to
342), fenced code blocks are missing language specifiers which triggers
markdownlint; update each triple-backtick block to include a language token such
as text or plaintext (e.g., ```text) so the examples are properly annotated
while leaving the block contents unchanged.

</details>  

This comment was marked as abuse.

Sign in to view

undead2146 force-pushed the test/pr-subscription-feature branch from 8032d25 to 168fac2 Compare December 14, 2025 19:34

This comment was marked as spam.

Sign in to view

undead2146 force-pushed the test/pr-subscription-feature branch from 168fac2 to e14c8bc Compare December 14, 2025 20:12

This comment was marked as spam.

Sign in to view

undead2146 force-pushed the test/pr-subscription-feature branch from e14c8bc to 712967a Compare December 14, 2025 20:25

This comment was marked as abuse.

Sign in to view

undead2146 force-pushed the test/pr-subscription-feature branch from 712967a to 53b265b Compare December 14, 2025 22:07

This comment was marked as spam.

Sign in to view

undead2146 force-pushed the test/pr-subscription-feature branch from 53b265b to f88b69e Compare December 14, 2025 23:23

This comment was marked as spam.

Sign in to view

undead2146 force-pushed the test/pr-subscription-feature branch from f88b69e to e017b38 Compare December 14, 2025 23:49

This comment was marked as spam.

Sign in to view

undead2146 force-pushed the test/pr-subscription-feature branch from e017b38 to e1814ce Compare December 15, 2025 00:10

This comment was marked as abuse.

Sign in to view

undead2146 force-pushed the test/pr-subscription-feature branch from e1814ce to 669d18f Compare December 15, 2025 00:22

This comment was marked as spam.

Sign in to view

undead2146 force-pushed the test/pr-subscription-feature branch from 669d18f to 24b5043 Compare December 15, 2025 00:30

This comment was marked as spam.

Sign in to view

Repository owner deleted a comment from netlify bot Dec 15, 2025

undead2146 force-pushed the test/pr-subscription-feature branch from 8554331 to 57ea8f9 Compare December 15, 2025 03:30

This comment was marked as duplicate.

Sign in to view

undead2146 force-pushed the test/pr-subscription-feature branch from 57ea8f9 to 50d9f84 Compare December 15, 2025 03:48

This comment was marked as abuse.

Sign in to view

undead2146 force-pushed the test/pr-subscription-feature branch from 50d9f84 to ce78086 Compare December 15, 2025 04:05

This comment was marked as abuse.

Sign in to view

undead2146 force-pushed the test/pr-subscription-feature branch from ce78086 to 79c22e5 Compare December 15, 2025 12:09

This comment was marked as spam.

Sign in to view

undead2146 force-pushed the test/pr-subscription-feature branch from 79c22e5 to e820868 Compare December 15, 2025 12:24

undead2146 force-pushed the test/pr-subscription-feature branch 3 times, most recently from ad3277b to 3283582 Compare December 16, 2025 20:39

coderabbitai bot reviewed Dec 16, 2025

View reviewed changes

undead2146 force-pushed the test/pr-subscription-feature branch 3 times, most recently from 666cac1 to 4815bb5 Compare December 16, 2025 21:10

coderabbitai bot reviewed Dec 16, 2025

View reviewed changes

undead2146 force-pushed the test/pr-subscription-feature branch from 4815bb5 to a28a068 Compare December 16, 2025 21:21

undead2146 force-pushed the main branch from 06eddfc to af7fbbd Compare December 16, 2025 21:25

coderabbitai bot reviewed Dec 16, 2025

View reviewed changes

undead2146 force-pushed the main branch from af7fbbd to 06eddfc Compare December 16, 2025 22:14

undead2146 force-pushed the test/pr-subscription-feature branch from a28a068 to 16824af Compare December 17, 2025 13:06

temp

37cacfd

undead2146 force-pushed the test/pr-subscription-feature branch from 16824af to 37cacfd Compare December 17, 2025 13:07

coderabbitai bot reviewed Dec 17, 2025

View reviewed changes

undead2146 force-pushed the main branch from 739fa3b to e08ffb9 Compare December 20, 2025 14:55

undead2146 force-pushed the main branch 12 times, most recently from 5203ea6 to c9ae161 Compare December 31, 2025 23:04

feat: Add application update functionality with Velopack #3

Are you sure you want to change the base?

feat: Add application update functionality with Velopack #3

Uh oh!

Conversation

undead2146 commented Dec 14, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as duplicate.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as spam.

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

undead2146 commented Dec 14, 2025 •

edited by coderabbitai bot

Loading