Feat/map manager #11
Feat/map manager #11
Conversation
…ommunity-outpost#234) Removed old local content dialog properties and methods from GameProfileSettingsViewModel. Enhanced UI with drag-and-drop support for file and folder imports.
✅ Deploy Preview for generalshub ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedToo many files! 134 files out of 284 files are above the max files limit of 150. You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds publisher/theme color metadata and threads progress reporting through content download, extraction, and manifest storage; introduces a GitHub manifest factory; adds a local-content import UI/VM; updates many call sites for the new AddManifestAsync progress parameter; and relaxes CI token handling for fork PRs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GitHubDeliverer as GitHub Deliverer
participant Downloader as File Downloader
participant Extractor as ZIP Extractor
participant ManifestPool as Manifest Pool
participant CAS as CAS Storage
participant Progress as Progress Reporter
rect rgb(240,248,255)
Client->>GitHubDeliverer: DeliverContentAsync(manifest, progress)
GitHubDeliverer->>Downloader: Download files (per-file)
Downloader->>Progress: Report Downloading (index/total, speed)
end
rect rgb(230,255,230)
GitHubDeliverer->>Extractor: Open ZIP & extract entries
Extractor->>Progress: Report Extraction (mapped percent range)
end
rect rgb(255,250,230)
GitHubDeliverer->>ManifestPool: AddManifestAsync(manifest, manifestDir, progress: storageProgress)
ManifestPool->>CAS: StoreContentAsync(files, progress: storageProgress)
CAS->>Progress: Report StoringInCas (percent, fileIndex)
ManifestPool-->>GitHubDeliverer: Store result
end
GitHubDeliverer-->>Client: Delivery complete (result)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
GenHub/GenHub/Features/Downloads/ViewModels/PublisherCardViewModel.cs (1)
622-639: Bug: Logging uses wrong variable in foreach loop.The logging statements on lines 627-637 reference
manifest.Id(the outer variable from line 572) instead ofm(the loop variable). This will log the same manifest ID for all iterations instead of each variant's ID.🔎 Proposed fix
foreach (var m in justInstalledGameClients) { var profileResult = await _profileService.CreateProfileFromManifestAsync(m); if (profileResult.Success) { _logger.LogInformation( "Created profile for {ManifestId}: {ProfileName}", - manifest.Id, + m.Id, profileResult.Data?.Name); } else { _logger.LogWarning( "Failed to create profile for {ManifestId}: {Errors}", - manifest.Id, + m.Id, string.Join(", ", profileResult.Errors)); } }
🧹 Nitpick comments (23)
GenHub/GenHub/Features/GameProfiles/ViewModels/FileTreeItem.cs (1)
28-28: Consider using explicit ObservableCollection initialization for better compatibility.The collection expression
[]syntax may not work reliably withObservableCollection<T>depending on the C# compiler version. The traditional initialization is more explicit and guaranteed to work.🔎 Proposed fix
- public ObservableCollection<FileTreeItem> Children { get; set; } = []; + public ObservableCollection<FileTreeItem> Children { get; set; } = new ObservableCollection<FileTreeItem>();Based on relevant code snippets provided.
GenHub/GenHub/Features/UserData/Services/UserDataTrackerService.cs (1)
712-731: Extract duplicated helper to a shared utility class.This method is duplicated in
WorkspaceStrategyBase.cs(lines 632-644). Consider extracting it to a shared utility class to follow the DRY principle and ensure consistent behavior across the codebase.Optional: Address minor return value inconsistency
When the prefix doesn't match, the method returns the original
pathrather than thenormalizedversion. This could result in inconsistent path separators (mixed forward/back slashes) in the returned values. Consider returning the normalized path in both cases for consistency:private static string StripLeadingDirectory(string path, string directoryName) { // Handle both forward and back slashes var normalized = path.Replace('\\', '/'); var prefix = directoryName + "/"; if (normalized.StartsWith(prefix, StringComparison.OrdinalIgnoreCase)) { return normalized[prefix.Length..]; } - return path; + return normalized; }GenHub/GenHub/Features/Content/Services/Helpers/GitHubInferenceHelper.cs (1)
53-61: Logic is correct; consider aligning with InferGameTypeFromAsset for consistency.The priority-based approach works correctly: zero hour indicators are checked first with early return, so the generals check at line 60 only executes when no zero hour indicators are present. This simplifies the previous implementation by eliminating redundant exclusion checks.
However,
InferGameTypeFromAsset(lines 156-159) uses an explicit exclusion approach for the generals check. For maintainability, consider aligning both methods to use the same pattern—either priority-based (current) or explicit exclusion (like InferGameTypeFromAsset).Optional: Align with explicit exclusion pattern
If you prefer consistency with
InferGameTypeFromAsset:// Check for Generals indicators -if (searchText.Contains("generals", StringComparison.OrdinalIgnoreCase)) +if (searchText.Contains("generals", StringComparison.OrdinalIgnoreCase) && + !searchText.Contains("zero hour", StringComparison.OrdinalIgnoreCase) && + !searchText.Contains("zh", StringComparison.OrdinalIgnoreCase) && + !searchText.Contains("zerohour", StringComparison.OrdinalIgnoreCase)) return (Type: GameType.Generals, IsInferred: true);Note: This is purely stylistic; the current implementation is functionally correct.
GenHub/GenHub/Common/ViewModels/MainViewModel.cs (2)
90-108: Consider documenting the Tools tab exclusion logic.The redirect from
NavigationTab.ToolstoGameProfileson initial load is intentional but not explained. A brief comment would clarify the reasoning for future maintainers.🔎 Suggested documentation
private static NavigationTab LoadInitialTab(IConfigurationProviderService configurationProvider, ILogger<MainViewModel>? logger) { try { var tab = configurationProvider.GetLastSelectedTab(); + // Tools tab should not be restored on startup (e.g., requires explicit user navigation) if (tab == NavigationTab.Tools) { tab = NavigationTab.GameProfiles; }
277-293: Fire-and-forget on SaveAsync may lose errors silently.Line 286 discards the
TaskfromSaveAsync(), meaning any async exceptions will be unobserved. For a low-priority preference like tab selection, this is acceptable, but consider logging failures if save reliability becomes important.🔎 Optional: Add async error handling
- _ = userSettingsService.SaveAsync(); + _ = userSettingsService.SaveAsync().ContinueWith( + t => logger?.LogWarning(t.Exception, "Failed to persist tab selection"), + TaskContinuationOptions.OnlyOnFaulted);GenHub/GenHub/Features/Workspace/Strategies/WorkspaceStrategyBase.cs (1)
629-645: Consider consistent path separator handling.The method returns paths with forward slashes when the prefix is stripped (line 641) but returns the original path with potentially mixed separators when the prefix doesn't match (line 644). While
Path.Combinehandles both separators on Windows, consistent behavior would be clearer.🔎 Suggested fix for consistent path separators
private static string StripLeadingDirectory(string path, string directoryName) { // Handle both forward and back slashes var normalized = path.Replace('\\', '/'); var prefix = directoryName + "/"; if (normalized.StartsWith(prefix, StringComparison.OrdinalIgnoreCase)) { return normalized[prefix.Length..]; } - return path; + return normalized; }GenHub/GenHub/Features/Downloads/Views/PublisherCardView.axaml (1)
209-209: Consider binding the progress bar color to the theme color from metadata.This PR introduces
ThemeColormetadata for content theming, but the progress bar still uses a hardcoded color. For consistency with the theming approach, consider binding theForegroundproperty to theThemeColorfrom the content item's metadata (if available), with the current color as a fallback.💡 Suggested approach
You could create a converter that extracts
ThemeColorfrom the ContentItem's metadata and use it like:<ProgressBar Value="{Binding DownloadProgress}" Maximum="100" Height="4" IsVisible="{Binding IsDownloading}" - Foreground="#A15DF3" + Foreground="{Binding ThemeColor, FallbackValue='#A15DF3'}" Background="#333333" />This would make the progress bar color dynamic based on the publisher's theme color defined in constants like
CommunityOutpostConstants.ThemeColor.GenHub/GenHub/Features/Content/Services/CommunityOutpost/CommunityOutpostDeliverer.cs (1)
273-277: Consider wiring up storage progress reporting.The PR introduces storage progress reporting (
ContentStorageProgress), but this call passesnullfor the progress parameter. SinceDeliverContentAsyncalready receives anIProgress<ContentAcquisitionProgress>parameter, consider creating an adapter to forward storage progress to the caller. This would provide better visibility during the "Registering manifests to content library" phase.💡 Example approach
+// Create progress adapter to map storage progress to acquisition progress +IProgress<ContentStorageProgress>? storageProgress = null; +if (progress != null) +{ + storageProgress = new Progress<ContentStorageProgress>(sp => + { + progress.Report(new ContentAcquisitionProgress + { + Phase = ContentAcquisitionPhase.Copying, + ProgressPercentage = 70 + (int)(sp.ProgressPercentage * 0.2), + CurrentOperation = sp.CurrentOperation ?? "Storing content", + }); + }); +} + var addResult = await manifestPool.AddManifestAsync( manifest, extractPath, - null, + storageProgress, cancellationToken);GenHub/GenHub/Features/Content/Services/ContentOrchestrator.cs (1)
531-531: Consider forwarding storage progress for better user feedback.Similar to other content deliverers, this call doesn't forward progress during manifest storage. Since the orchestrator already has a
progressparameter and is at the 80% mark (extracting phase), forwarding storage progress would provide users with visibility into the "Adding to content library" operation.💡 Suggested approach
+// Forward storage progress to acquisition progress +IProgress<ContentStorageProgress>? storageProgress = null; +if (progress != null) +{ + storageProgress = new Progress<ContentStorageProgress>(sp => + { + var pct = ContentConstants.ProgressStepExtracting + (int)(sp.ProgressPercentage * 0.15); + progress.Report(new ContentAcquisitionProgress + { + Phase = ContentAcquisitionPhase.Extracting, + ProgressPercentage = pct, + CurrentOperation = sp.CurrentOperation ?? "Storing in library", + }); + }); +} + -await _manifestPool.AddManifestAsync(prepareResult.Data, stagingDir, cancellationToken: cancellationToken); +await _manifestPool.AddManifestAsync(prepareResult.Data, stagingDir, storageProgress, cancellationToken);GenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileLauncherViewModel.ManifestHelpers.cs (1)
125-125: Consider wiring progress reporting for better UX.The call correctly adapts to the new
AddManifestAsyncsignature by passingnullfor progress. However, since this is a user-facing manual installation flow, reporting progress could improve the experience when registering large installations.If this ViewModel has access to a progress reporter (e.g., via a property or parameter), consider passing it through instead of
null.GenHub/GenHub/Features/Manifest/ManifestProvider.cs (1)
96-96: LGTM! Progress parameter consistently set to null.All four
AddManifestAsynccall sites correctly passnullfor the new progress parameter. This is acceptable for ManifestProvider operations, which are typically fast internal operations.For future enhancement, consider whether progress reporting would be beneficial for long-running manifest addition operations, particularly when adding large generated manifests with many files.
Also applies to: 154-154, 208-208, 264-264
GenHub/GenHub/Features/Content/Services/Publishers/GitHubManifestFactory.cs (2)
61-87: Verify hash computation cancellation and error handlingThe loop checks
cancellationToken.IsCancellationRequestedat Line 63 but then callsComputeFileHashAsyncat Line 70 without ensuring that exceptions from file I/O or hash computation are handled gracefully. Consider wrapping the file processing in a try-catch block to handle scenarios where file access fails (e.g., locked files, permission issues).Additionally, the
IsRequired = trueassignment at Line 82 marks all files as required. This may not be appropriate for all files in extracted archives—consider whether some files should be optional.🔎 Suggested error handling enhancement
foreach (var filePath in allFiles) { if (cancellationToken.IsCancellationRequested) break; - var relativePath = Path.GetRelativePath(extractedDirectory, filePath); - var fileInfo = new FileInfo(filePath); - - // Compute hash for ContentAddressable storage - string fileHash = await hashProvider.ComputeFileHashAsync(filePath, cancellationToken); - - // Determine if executable (simple heuristic for now, can be improved) - bool isExecutable = filePath.EndsWith(".exe", StringComparison.OrdinalIgnoreCase) || - filePath.EndsWith(".bat", StringComparison.OrdinalIgnoreCase) || - filePath.EndsWith(".cmd", StringComparison.OrdinalIgnoreCase); - - files.Add(new ManifestFile - { - RelativePath = relativePath, - Size = fileInfo.Length, - Hash = fileHash, - IsRequired = true, - IsExecutable = isExecutable, - SourceType = ContentSourceType.ContentAddressable, - SourcePath = filePath, - }); + try + { + var relativePath = Path.GetRelativePath(extractedDirectory, filePath); + var fileInfo = new FileInfo(filePath); + + // Compute hash for ContentAddressable storage + string fileHash = await hashProvider.ComputeFileHashAsync(filePath, cancellationToken); + + // Determine if executable (simple heuristic for now, can be improved) + bool isExecutable = filePath.EndsWith(".exe", StringComparison.OrdinalIgnoreCase) || + filePath.EndsWith(".bat", StringComparison.OrdinalIgnoreCase) || + filePath.EndsWith(".cmd", StringComparison.OrdinalIgnoreCase); + + files.Add(new ManifestFile + { + RelativePath = relativePath, + Size = fileInfo.Length, + Hash = fileHash, + IsRequired = true, + IsExecutable = isExecutable, + SourceType = ContentSourceType.ContentAddressable, + SourcePath = filePath, + }); + } + catch (Exception ex) + { + logger.LogWarning(ex, "Failed to process file {FilePath}, skipping", filePath); + // Continue processing other files + } }
73-75: Consider cross-platform executability detectionThe executability heuristic only checks for Windows file extensions (.exe, .bat, .cmd). On Unix-based systems, executables may not have extensions and rely on file permissions. While this may be acceptable for the current use case (C&C Generals runs primarily on Windows), consider documenting this limitation or enhancing the detection logic for broader cross-platform support.
GenHub/GenHub/Features/Content/Services/GitHub/GitHubContentDeliverer.cs (1)
141-203: Verify manual ZIP extraction necessity and add error handlingThe code replaces the simpler
ZipFile.ExtractToDirectorywith manual entry-by-entry extraction (Lines 152-183). While this enables per-file progress reporting, it lacks error handling for individual entry extraction failures. If an entry fails to extract (e.g., due to I/O errors or path issues), the entire operation will fail rather than logging and continuing.Additionally, verify that the progress range alignment is correct: downloads end at 65%, extraction runs 65-70%, leaving 70-90% unaccounted for before storage at 90%.
🔎 Suggested error handling enhancement
foreach (var entry in archive.Entries) { if (string.IsNullOrEmpty(entry.Name)) continue; // Skip directories - var destinationPath = Path.Combine(targetDirectory, entry.FullName); - var destinationDir = Path.GetDirectoryName(destinationPath); - if (!string.IsNullOrEmpty(destinationDir)) Directory.CreateDirectory(destinationDir); - - entry.ExtractToFile(destinationPath, overwrite: true); - currentEntry++; + try + { + var destinationPath = Path.Combine(targetDirectory, entry.FullName); + var destinationDir = Path.GetDirectoryName(destinationPath); + if (!string.IsNullOrEmpty(destinationDir)) Directory.CreateDirectory(destinationDir); + + entry.ExtractToFile(destinationPath, overwrite: true); + currentEntry++; + } + catch (Exception ex) + { + logger.LogWarning(ex, "Failed to extract entry {EntryName}, skipping", entry.FullName); + continue; + } // Map extraction progress from 65% to 70% double extractStart = 65;GenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileSettingsViewModel.cs (2)
99-101: Consider unregistering from messages to prevent memory leaks.The ViewModel registers with
WeakReferenceMessenger.Default.Register(this)but there's no corresponding unregistration when the ViewModel is disposed or the window closes. WhileWeakReferenceMessengeruses weak references, explicit unregistration is a best practice, especially if the ViewModel outlives expectations.🔎 Proposed fix
Consider implementing
IDisposableor adding cleanup logic:// Add a cleanup method or implement IDisposable public void Cleanup() { WeakReferenceMessenger.Default.Unregister<Core.Models.Content.ContentAcquiredMessage>(this); }
504-512: Fire-and-forget async call should handle exceptions.The
Receivemethod discards the task fromLoadAvailableContentAsync()using_ =. If an exception occurs during content loading, it will be unobserved. Consider wrapping in a try-catch or using a helper method.🔎 Proposed improvement
public void Receive(Core.Models.Content.ContentAcquiredMessage message) { // Refresh available content when new content is acquired - _ = LoadAvailableContentAsync(); + _ = SafeLoadAvailableContentAsync(); +} + +private async Task SafeLoadAvailableContentAsync() +{ + try + { + await LoadAvailableContentAsync(); + } + catch (Exception ex) + { + logger?.LogError(ex, "Error refreshing content after acquisition"); + } }GenHub/GenHub/Features/GameProfiles/Views/AddLocalContentWindow.axaml.cs (2)
27-66: Event subscription without cleanup may cause memory leak.The
RequestCloseevent subscription on line 32 is never unsubscribed. While this may not be critical if the window and ViewModel are disposed together, it's safer to unsubscribe when the window is closed.🔎 Proposed fix
protected override void OnDataContextChanged(EventArgs e) { base.OnDataContextChanged(e); if (DataContext is AddLocalContentViewModel vm) { - vm.RequestClose += (s, result) => Close(result); + vm.RequestClose += OnRequestClose; // Wire up the browse delegates vm.BrowseFolderAction = async () => { // ... existing code }; } } + +private void OnRequestClose(object? sender, bool result) +{ + if (DataContext is AddLocalContentViewModel vm) + { + vm.RequestClose -= OnRequestClose; + } + Close(result); +}
86-101: Consider adding error handling for robustness.While
ImportContentAsynchas internal exception handling, wrapping the loop in a try-catch would provide defense-in-depth against unexpected exceptions during file enumeration.🔎 Proposed fix
private async void OnDrop(object? sender, DragEventArgs e) { if (DataContext is not AddLocalContentViewModel vm) return; - var files = e.Data.GetFiles(); - if (files != null) + try { - foreach (var file in files) + var files = e.Data.GetFiles(); + if (files != null) { - if (file?.Path?.LocalPath is { } path) + foreach (var file in files) { - await vm.ImportContentAsync(path); + if (file?.Path?.LocalPath is { } path) + { + await vm.ImportContentAsync(path); + } } } } + catch (Exception ex) + { + // Log or handle unexpected errors during drop operation + System.Diagnostics.Debug.WriteLine($"Drop error: {ex.Message}"); + } }GenHub/GenHub/Features/GameProfiles/Views/AddLocalContentWindow.axaml (2)
82-89: Use static reference for type-safe ContentType comparison.The
ConverterParameter=GameClientis a string comparison which could silently fail if the enum value name changes. Use a static reference for type safety, consistent with other bindings in this file.🔎 Proposed fix
<!-- Target Game --> -<StackPanel Spacing="8" IsVisible="{Binding SelectedContentType, Converter={x:Static ObjectConverters.Equal}, ConverterParameter=GameClient}"> +<StackPanel Spacing="8" IsVisible="{Binding SelectedContentType, Converter={x:Static ObjectConverters.Equal}, ConverterParameter={x:Static models:ContentType.GameClient}}"> <TextBlock Text="TARGET GAME" Classes="label"/>Note: You'll need to add the models namespace if not already present:
xmlns:models="clr-namespace:GenHub.Core.Models.Enums;assembly=GenHub.Core"
156-160: Consider using a clearer empty-state binding.The
!FileTree.Countnegation syntax works but relies on implicit int-to-bool conversion. For improved readability, consider using a converter or anIsEmptyproperty on the ViewModel.🔎 Alternative approaches
Option 1 - Use existing converter:
IsVisible="{Binding FileTree.Count, Converter={x:Static ObjectConverters.Equal}, ConverterParameter=0}"Option 2 - Add a property to the ViewModel:
public bool IsFileTreeEmpty => !FileTree.Any();GenHub/GenHub/Features/GameProfiles/ViewModels/AddLocalContentViewModel.cs (3)
213-234: Truncation limits may confuse users with large imports.The
Take(20)andTake(50)limits silently truncate the preview tree without user notification. Users importing large content packs may not realize files are hidden.🔎 Proposed improvement
Consider adding a visual indicator when items are truncated:
foreach (var d in dir.GetDirectories().Take(20)) { items.Add(new FileTreeItem { /* ... */ }); } + +var remainingDirs = dir.GetDirectories().Length - items.Count(i => !i.IsFile); +if (remainingDirs > 0) +{ + items.Add(new FileTreeItem { Name = $"... and {remainingDirs} more folders", IsFile = false }); +} foreach (var f in dir.GetFiles().Take(50)) { items.Add(new FileTreeItem { Name = f.Name, IsFile = true, FullPath = f.FullName }); } + +var remainingFiles = dir.GetFiles().Length - items.Count(i => i.IsFile); +if (remainingFiles > 0) +{ + items.Add(new FileTreeItem { Name = $"... and {remainingFiles} more files", IsFile = true }); +}
398-411: Empty catch block swallows cleanup errors silently.Even for non-critical cleanup, logging the error aids debugging when temp directories accumulate.
🔎 Proposed fix
private void CleanupStaging() { try { if (Directory.Exists(_stagingPath)) { Directory.Delete(_stagingPath, true); } } - catch + catch (Exception ex) { - // Ignore cleanup errors + logger?.LogWarning(ex, "Failed to cleanup staging directory: {Path}", _stagingPath); } }
278-316: Synchronous file operations could freeze UI for large directories.
File.DeleteandDirectory.Deleteat lines 295-299 are blocking operations. Consider wrapping them inTask.Runlike other file operations in this class.🔎 Proposed fix
-if (item.IsFile && File.Exists(item.FullPath)) -{ - File.Delete(item.FullPath); -} -else if (!item.IsFile && Directory.Exists(item.FullPath)) -{ - Directory.Delete(item.FullPath, true); -} +await Task.Run(() => +{ + if (item.IsFile && File.Exists(item.FullPath)) + { + File.Delete(item.FullPath); + } + else if (!item.IsFile && Directory.Exists(item.FullPath)) + { + Directory.Delete(item.FullPath, true); + } +});
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
GenHub/GenHub.Core/Constants/CommunityOutpostConstants.csGenHub/GenHub.Core/Constants/ContentConstants.csGenHub/GenHub.Core/Constants/GeneralsOnlineConstants.csGenHub/GenHub.Core/Constants/SuperHackersConstants.csGenHub/GenHub.Core/Interfaces/GameProfiles/IGameClientProfileService.csGenHub/GenHub.Core/Interfaces/Manifest/IContentManifestPool.csGenHub/GenHub.Core/Models/Content/ContentAcquiredMessage.csGenHub/GenHub.Core/Models/Content/ContentAcquisitionPhase.csGenHub/GenHub.Core/Models/Enums/ContentType.csGenHub/GenHub.Core/Models/Manifest/ContentMetadata.csGenHub/GenHub.Core/Services/Content/LocalContentService.csGenHub/GenHub.Tests/GenHub.Tests.Core/Features/Content/ContentOrchestratorTests.csGenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameClients/GameClientDetectorTests.csGenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameClients/GameClientManifestIntegrationTests.csGenHub/GenHub/Common/ViewModels/MainViewModel.csGenHub/GenHub/Features/Content/Services/CommunityOutpost/CommunityOutpostDeliverer.csGenHub/GenHub/Features/Content/Services/CommunityOutpost/CommunityOutpostManifestFactory.csGenHub/GenHub/Features/Content/Services/ContentOrchestrator.csGenHub/GenHub/Features/Content/Services/GeneralsOnline/GeneralsOnlineDeliverer.csGenHub/GenHub/Features/Content/Services/GeneralsOnline/GeneralsOnlineManifestFactory.csGenHub/GenHub/Features/Content/Services/GitHub/GitHubContentDeliverer.csGenHub/GenHub/Features/Content/Services/Helpers/GitHubInferenceHelper.csGenHub/GenHub/Features/Content/Services/Publishers/GitHubManifestFactory.csGenHub/GenHub/Features/Content/Services/Publishers/SuperHackersManifestFactory.csGenHub/GenHub/Features/Downloads/ViewModels/PublisherCardViewModel.csGenHub/GenHub/Features/Downloads/Views/PublisherCardView.axamlGenHub/GenHub/Features/GameInstallations/GameInstallationService.csGenHub/GenHub/Features/GameProfiles/Services/GameClientProfileService.csGenHub/GenHub/Features/GameProfiles/Services/ProfileLauncherFacade.csGenHub/GenHub/Features/GameProfiles/ViewModels/AddLocalContentViewModel.csGenHub/GenHub/Features/GameProfiles/ViewModels/FileTreeItem.csGenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileLauncherViewModel.ManifestHelpers.csGenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileSettingsViewModel.csGenHub/GenHub/Features/GameProfiles/Views/AddLocalContentWindow.axamlGenHub/GenHub/Features/GameProfiles/Views/AddLocalContentWindow.axaml.csGenHub/GenHub/Features/GameProfiles/Views/GameProfileSettingsWindow.axamlGenHub/GenHub/Features/Manifest/ContentManifestPool.csGenHub/GenHub/Features/Manifest/ManifestProvider.csGenHub/GenHub/Features/UserData/Services/UserDataTrackerService.csGenHub/GenHub/Features/Workspace/Strategies/WorkspaceStrategyBase.csGenHub/GenHub/Infrastructure/DependencyInjection/ContentPipelineModule.cs
🧰 Additional context used
🧬 Code graph analysis (10)
GenHub/GenHub/Features/Content/Services/Publishers/SuperHackersManifestFactory.cs (1)
GenHub/GenHub.Core/Constants/SuperHackersConstants.cs (1)
SuperHackersConstants(6-108)
GenHub/GenHub/Features/UserData/Services/UserDataTrackerService.cs (4)
GenHub/GenHub/Features/Content/Services/CommunityOutpost/CommunityOutpostManifestFactory.cs (1)
ContentInstallTarget(156-198)GenHub/GenHub/Features/Manifest/ContentManifestBuilder.cs (1)
ContentInstallTarget(753-782)GenHub/GenHub.Core/Constants/GameSettingsConstants.cs (2)
GameSettingsConstants(6-181)FolderNames(110-141)GenHub/GenHub/Features/Workspace/Strategies/WorkspaceStrategyBase.cs (1)
StripLeadingDirectory(633-645)
GenHub/GenHub/Features/Content/Services/CommunityOutpost/CommunityOutpostManifestFactory.cs (1)
GenHub/GenHub.Core/Constants/CommunityOutpostConstants.cs (1)
CommunityOutpostConstants(11-102)
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameClients/GameClientManifestIntegrationTests.cs (1)
GenHub/GenHub.Core/Models/Manifest/ContentManifest.cs (1)
ContentManifest(10-53)
GenHub/GenHub/Features/Downloads/ViewModels/PublisherCardViewModel.cs (2)
GenHub/GenHub/Features/Manifest/ContentManifestBuilder.cs (1)
ContentManifest(693-724)GenHub/GenHub.Core/Models/Manifest/ContentManifest.cs (1)
ContentManifest(10-53)
GenHub/GenHub/Features/Content/Services/Publishers/GitHubManifestFactory.cs (5)
GenHub/GenHub/Features/Content/Services/Publishers/PublisherManifestFactoryResolver.cs (1)
IPublisherManifestFactory(19-41)GenHub/GenHub/Features/Content/Services/Publishers/SuperHackersManifestFactory.cs (2)
CanHandle(46-55)GetManifestDirectory(100-112)GenHub/GenHub.Core/Interfaces/GameProfiles/IGameClientProfileService.cs (4)
Task(26-32)Task(46-52)Task(61-63)Task(71-73)GenHub/GenHub.Core/Interfaces/Manifest/IContentManifestPool.cs (8)
Task(20-20)Task(30-30)Task(38-38)Task(45-45)Task(53-53)Task(61-61)Task(69-69)Task(77-77)GenHub/GenHub.Core/Constants/PublisherInfoConstants.cs (1)
Name(121-133)
GenHub/GenHub/Features/Content/Services/GeneralsOnline/GeneralsOnlineManifestFactory.cs (1)
GenHub/GenHub.Core/Constants/GeneralsOnlineConstants.cs (1)
GeneralsOnlineConstants(6-125)
GenHub/GenHub/Features/GameProfiles/Services/ProfileLauncherFacade.cs (1)
GenHub/GenHub.Core/Constants/PublisherInfoConstants.cs (1)
Name(121-133)
GenHub/GenHub/Features/GameProfiles/Services/GameClientProfileService.cs (8)
GenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileLauncherViewModel.cs (1)
GetThemeColorForGameType(310-313)GenHub/GenHub/Features/Content/Services/Helpers/GitHubInferenceHelper.cs (2)
GameType(49-64)GameType(135-162)GenHub/GenHub.Core/Models/GameClients/GameClient.cs (1)
GameClient(8-107)GenHub/GenHub.Core/Constants/PublisherTypeConstants.cs (1)
PublisherTypeConstants(21-59)GenHub/GenHub.Core/Constants/SuperHackersConstants.cs (1)
SuperHackersConstants(6-108)GenHub/GenHub.Core/Constants/PublisherInfoConstants.cs (1)
GeneralsOnline(104-114)GenHub/GenHub.Core/Constants/GeneralsOnlineConstants.cs (1)
GeneralsOnlineConstants(6-125)GenHub/GenHub.Core/Constants/CommunityOutpostConstants.cs (1)
CommunityOutpostConstants(11-102)
GenHub/GenHub/Features/GameProfiles/ViewModels/AddLocalContentViewModel.cs (3)
GenHub/GenHub/Features/GameProfiles/ViewModels/FileTreeItem.cs (1)
FileTreeItem(8-29)GenHub/GenHub.Core/Interfaces/Manifest/IContentManifestPool.cs (8)
Task(20-20)Task(30-30)Task(38-38)Task(45-45)Task(53-53)Task(61-61)Task(69-69)Task(77-77)GenHub/GenHub.Core/Services/Content/LocalContentService.cs (1)
Task(41-131)
⏰ 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 (48)
GenHub/GenHub/Features/UserData/Services/UserDataTrackerService.cs (1)
705-707: LGTM! Path normalization prevents double-nesting.The use of
StripLeadingDirectorycorrectly prevents double-nesting of folder names (e.g., "Maps/Maps/file.map") when combining relative paths with the base directory that already includes the target folder name.GenHub/GenHub/Common/ViewModels/MainViewModel.cs (3)
45-88: Well-structured primary constructor with DI.The refactor to C# 12 primary constructor syntax is clean and properly encapsulates dependencies. Field initialization via
LoadInitialTabis safe since the parameters are guaranteed non-null at that point.
171-180: LGTM!The logger usage is consistent with the primary constructor pattern, and the fire-and-forget for background updates is appropriate since
CheckForUpdatesInBackgroundAsynchandles its own exceptions.
195-275: LGTM!The dependency reference updates are consistent with the primary constructor pattern. The dual exception handling (in
CheckForUpdatesAsyncand its wrapper) provides robust error isolation for background operations.GenHub/GenHub.Core/Models/Content/ContentAcquisitionPhase.cs (1)
43-46: LGTM! Clear and well-documented enum addition.The new
StoringInCasphase is logically placed in the acquisition workflow and has clear documentation. This aligns well with the progress reporting enhancements mentioned in the summary.GenHub/GenHub/Features/Workspace/Strategies/WorkspaceStrategyBase.cs (1)
475-477: LGTM! Path resolution correctly prevents directory duplication.The updated logic properly strips leading directory names (Maps, Replays, Screenshots) from relative paths before combining with the target base path, preventing paths like
Maps/Maps/mymap.map.GenHub/GenHub.Core/Constants/SuperHackersConstants.cs (1)
38-46: LGTM! Theme color constants are well-defined.The new theme color constants are properly documented and logically placed with other UI metadata. The color values (dark red for Zero Hour, orange for Generals) provide clear visual differentiation between the variants.
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Content/ContentOrchestratorTests.cs (2)
114-114: LGTM! Test mock correctly updated for new signature.The mock setup properly includes the new
IProgress<ContentStorageProgress>parameter added toAddManifestAsync, ensuring the test reflects the updated interface.
132-132: LGTM! Verification correctly validates the new signature.The verification properly checks that
AddManifestAsyncis called with all required parameters including the new progress reporter.GenHub/GenHub/Infrastructure/DependencyInjection/ContentPipelineModule.cs (1)
154-157: Multiple IPublisherManifestFactory registrations are correctly handled.The
PublisherManifestFactoryResolver(line 22) usesFirstOrDefault(f => f.CanHandle(manifest))to select the appropriate factory. BothSuperHackersManifestFactoryandGitHubManifestFactoryimplement distinctCanHandlelogic:SuperHackersManifestFactoryrequires the "thesuperhackers" publisher type AND GameClient content type (line 54), whileGitHubManifestFactoryrequires only the "github" publisher type (line 30). No conflicts exist between these registrations.GenHub/GenHub.Core/Constants/CommunityOutpostConstants.cs (1)
38-41: LGTM!The
ThemeColorconstant is properly documented and follows the same pattern used in other publisher constants. The dark green color (#2D5A27) appears appropriate for Community Outpost branding.GenHub/GenHub.Core/Models/Manifest/ContentMetadata.cs (1)
42-46: LGTM!The
ThemeColorproperty is a clean addition that extends the metadata model for theming support. The nullable string type is appropriate since theme colors are optional, and the XML documentation is clear.GenHub/GenHub/Features/Content/Services/GeneralsOnline/GeneralsOnlineDeliverer.cs (1)
139-139: LGTM! Named parameter improves code clarity.The refactoring to use named parameter binding (
cancellationToken: cancellationToken) makes the code more explicit and readable, especially when optional parameters are involved. The change is functionally equivalent and consistently applied across all three manifest registrations.Also applies to: 154-154, 169-169
GenHub/GenHub/Features/Content/Services/CommunityOutpost/CommunityOutpostManifestFactory.cs (1)
277-277: LGTM! Theme color properly integrated.The addition of
ThemeColorto the manifest metadata follows the established pattern used by other publishers and enhances the UI theming capabilities. The constant is correctly sourced fromCommunityOutpostConstants.ThemeColor.GenHub/GenHub.Core/Constants/ContentConstants.cs (1)
83-86: LGTM! Storage progress step properly added.The new
ProgressStepStoringconstant (90%) logically fits between the extracting (85%) and completion (100%) phases, providing better granularity for content acquisition progress reporting.GenHub/GenHub.Core/Constants/GeneralsOnlineConstants.cs (1)
39-42: LGTM! Theme color constant properly defined.The addition of
ThemeColorfollows the consistent pattern established across other publisher constants and uses a valid hex color code format. This enhances the theming support for Generals Online content.GenHub/GenHub.Core/Models/Content/ContentAcquiredMessage.cs (1)
1-9: LGTM! Clean message type implementation.The
ContentAcquiredMessagerecord is well-designed for event-driven messaging using modern C# patterns. The single-parameter design with clear documentation makes its purpose and usage straightforward.GenHub/GenHub/Features/Content/Services/Publishers/SuperHackersManifestFactory.cs (1)
360-360: LGTM! Theme color assignment is correctly conditioned on game type.The theme color selection properly differentiates Zero Hour (dark red
#8B0000) from Generals (orange#FFA500), consistent with the cover URL pattern on line 359.GenHub/GenHub/Features/Content/Services/GeneralsOnline/GeneralsOnlineManifestFactory.cs (1)
86-86: LGTM! Theme color consistently applied across all GeneralsOnline manifest variants.The
GeneralsOnlineConstants.ThemeColorvalue is properly propagated to:
- 30Hz and 60Hz game client variants
- QuickMatch MapPack manifest
- All manifest creation paths (initial, extracted, and local install)
The consistent use of a single theme color for the publisher is appropriate.
Also applies to: 313-313, 379-380, 405-406, 431-431
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameClients/GameClientDetectorTests.cs (1)
5-5: LGTM! Consistent test mock updates.All mock setups for
AddManifestAsynchave been correctly updated to include the newIProgress<ContentStorageProgress>parameter. The changes are consistent across all eight test methods.Also applies to: 97-98, 147-148, 189-190, 292-293, 385-386, 468-469, 562-563, 613-614
GenHub/GenHub.Core/Interfaces/Manifest/IContentManifestPool.cs (1)
27-30: LGTM! Well-designed interface extension.The addition of the optional
progressparameter with a default value ofnullis well-designed. This maintains backward compatibility while enabling progress reporting for storage operations. The documentation is clear and the parameter positioning (beforeCancellationToken) follows .NET conventions.GenHub/GenHub.Core/Interfaces/GameProfiles/IGameClientProfileService.cs (1)
23-32: LGTM! Consistent theming support added.The addition of the optional
themeColorparameter to both profile creation methods is well-designed and consistent. The parameter ordering (after visual elements like icon and cover) is logical, and the defaultnullvalue maintains backward compatibility.Also applies to: 43-52
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameClients/GameClientManifestIntegrationTests.cs (4)
27-27: LGTM! Appropriate for integration tests.Changing from
IFileHashProviderto the concreteSha256HashProvidertype is appropriate for integration tests, as it ensures deterministic SHA-256 hash computation. This provides more predictable test behavior compared to a mocked interface.
49-50: LGTM! Mock setup correctly updated.The mock setup for
AddManifestAsynchas been correctly updated to include the newIProgress<ContentStorageProgress>parameter, aligning with the interface change.
82-82: LGTM! Modern C# syntax improvements.Good use of C# 12 collection expressions and direct indexing. The change from
.First()to[0]is safe here since the test assertsSingle(result.Items)on line 85, ensuring exactly one item exists.Also applies to: 87-87
167-167: LGTM! Best practice for IDisposable pattern.Adding
GC.SuppressFinalize(this)is a best practice in the Dispose method, even though the class doesn't currently have a finalizer. This follows the standard IDisposable pattern and prevents potential issues if a finalizer is added in the future.GenHub/GenHub/Features/Manifest/ContentManifestPool.cs (2)
77-80: LGTM: Progress reporting integrationThe addition of the
IProgress<ContentStorageProgress>?parameter to the method signature is well-documented and follows the established pattern of optional progress reporting throughout the codebase.
102-102: LGTM: Progress propagation to storage serviceThe progress reporter is correctly passed through to
StoreContentAsync, enabling storage progress visibility.GenHub/GenHub/Features/GameInstallations/GameInstallationService.cs (1)
386-386: LGTM: Call site updated for new signatureThe call to
AddManifestAsynccorrectly passesnullfor the new progress parameter, which is appropriate for this manifest registration scenario where progress reporting is not needed.GenHub/GenHub/Features/GameProfiles/Services/ProfileLauncherFacade.cs (2)
861-871: LGTM: Optional dependency support implemented correctlyThe validation logic correctly respects the
IsOptionalflag, only adding errors for non-optional dependencies while maintaining observability through logging for all cases.
924-934: LGTM: Consistent optional dependency handlingAll dependency validation checks follow the same pattern: construct error messages, add to errors only for non-optional dependencies, and log with optional status. This ensures consistent behavior across all validation types (ID-based, version, game type, publisher type).
Also applies to: 944-956, 974-984, 994-1006, 1021-1033
GenHub/GenHub/Features/Content/Services/Publishers/GitHubManifestFactory.cs (3)
27-40: LGTM: Publisher matching logicThe
CanHandlemethod correctly identifies GitHub content by checking thePublisherType. The comments explain the rationale for broad acceptance as a fallback factory.
50-54: LGTM: Early validationChecking for directory existence before processing prevents unnecessary work and provides clear error handling through logging and early return.
90-108: LGTM: Manifest cloning preserves metadataThe manifest construction correctly preserves all original metadata (IDs, names, versions, dependencies, etc.) while replacing only the Files collection with the newly scanned content. This ensures consistency with the original manifest structure.
GenHub/GenHub/Features/Content/Services/GitHub/GitHubContentDeliverer.cs (2)
315-329: LGTM: Storage progress adapter correctly maps to acquisition progressThe storage progress adapter correctly maps
ContentStorageProgresstoContentAcquisitionProgressfor the StoringInCas phase, with progress ranging from 90-100%. The file count and current file information are properly propagated.
329-329: LGTM: Progress propagation to manifest poolThe
storageProgressadapter is correctly passed toAddManifestAsync, enabling storage progress visibility throughout the content acquisition pipeline.GenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileSettingsViewModel.cs (2)
32-32: LGTM on messaging interface implementation.The class correctly implements
IRecipient<ContentAcquiredMessage>to enable message-driven refresh of available content when new content is acquired.
2130-2150: LGTM on dialog result handling.The dialog flow correctly checks the result and the created content item before adding to the available content list and enabling it.
GenHub/GenHub/Features/GameProfiles/Services/GameClientProfileService.cs (3)
38-38: LGTM on theme color parameter addition.The optional
themeColorparameter is properly added with a null default, and the fallback logic on line 90 correctly uses the publisher-awareGetThemeColorForGameTypewhen no explicit color is provided.Also applies to: 90-90
368-394: Well-structured publisher-specific theming logic.The
GetThemeColorForGameTypemethod properly handles:
- TheSuperHackers with game-type-specific colors (ZeroHour vs Generals)
- GeneralsOnline with its dark blue theme
- CommunityOutpost with green theme
- Returns
nullfor unknown publishers to defer to manifest colorsThis aligns well with the constants defined in
SuperHackersConstants,GeneralsOnlineConstants, andCommunityOutpostConstantsfrom the relevant code snippets.
269-269: No action required. TheThemeColorproperty exists on theContentMetadataclass as a public nullable string property (GenHub/GenHub.Core/Models/Manifest/ContentMetadata.cs, line 46). The code correctly accesses this property, and it is properly integrated throughout the codebase.GenHub/GenHub/Features/Downloads/ViewModels/PublisherCardViewModel.cs (3)
390-394: LGTM on progress status formatting improvement.The early return when
CurrentOperationis non-empty provides more specific progress feedback to users, prioritizing operational details over generic phase information.
447-450: Good fix to restrict userVersion "0" skip to GameClient only.The guard now correctly only skips local client manifests (with userVersion "0") when the content type is
GameClient, preventing unintended filtering of other content types like MapPacks.
572-596: LGTM on improved download result handling.The pattern matching
result.Data is Core.Models.Manifest.ContentManifest manifestis cleaner and theContentAcquiredMessagedispatch is properly wrapped in try-catch to prevent download flow failures from messaging issues.GenHub/GenHub/Features/GameProfiles/Views/GameProfileSettingsWindow.axaml (1)
13-13: LGTM!The addition of
x:Name="SettingsWindow"and the refactoring to move the Add Local Content dialog to a separate window is a clean separation of concerns. The remaining code maintains consistent styling and structure.GenHub/GenHub/Features/GameProfiles/Views/AddLocalContentWindow.axaml (1)
1-21: Well-structured window implementation.Good use of Avalonia features including acrylic transparency, drag-drop support, and custom chrome. The two-column layout with form and preview pane provides clear UX.
GenHub/GenHub/Features/GameProfiles/ViewModels/AddLocalContentViewModel.cs (2)
16-46: Clean MVVM implementation with CommunityToolkit.Mvvm.Good use of primary constructor pattern,
[ObservableProperty]attributes, and[RelayCommand]for commands. The static property pattern for available types is appropriate.
135-211: Well-implemented import flow with proper async handling.The import logic correctly handles both ZIP archives and folders, uses
Task.Runfor blocking operations, and includes automatic map file organization. Progress and status updates provide good UX feedback.
| ContentType.Map, | ||
| ContentType.MapPack, | ||
| ContentType.Mission, | ||
| ContentType.Mod, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for ContentType.Mods usage and find relevant files
rg -nP --type=cs '\bContentType\.Mods\b' -C3Repository: undead2146/GenHub
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Find and read ContentType.cs file
fd -e cs "ContentType.cs" -x cat -n {}Repository: undead2146/GenHub
Length of output: 2283
🏁 Script executed:
#!/bin/bash
# Find and read LocalContentService.cs file, focusing on lines around 37 and 54
fd -e cs "LocalContentService.cs" -x head -n 70 {}Repository: undead2146/GenHub
Length of output: 3892
🏁 Script executed:
#!/bin/bash
# Search for Mods references in C# files to understand usage pattern
rg -nP --type=cs '\bMods\b' | head -30Repository: undead2146/GenHub
Length of output: 1071
Verify handling of the ContentType.Mods enum value.
ContentType.Mods is defined in ContentType.cs (line 25) as "Major gameplay changes (alias for Mod)" but is not included in AllowedContentTypes. The validation check will reject ContentType.Mods, creating an inconsistency. However, ContentType.Mods is not used anywhere in the codebase, suggesting the enum value is redundant.
Either:
- Remove
ContentType.Modsfrom the enum entirely (preferred—eliminate unused alias) - Add
ContentType.ModstoAllowedContentTypesif it must coexist withMod
Clarify the intended usage pattern.
🤖 Prompt for AI Agents
In GenHub/GenHub.Core/Services/Content/LocalContentService.cs around line 37 and
ContentType.cs at line 25, the enum value ContentType.Mods (alias for Mod) is
inconsistent because AllowedContentTypes does not include Mods; fix by removing
the redundant ContentType.Mods entry from ContentType.cs (preferred) and then
run a solution-wide search to remove/update any references or tests; if you must
keep the alias instead, add ContentType.Mods to AllowedContentTypes in
LocalContentService and ensure validation and tests accept it.
GenHub/GenHub/Features/Content/Services/GitHub/GitHubContentDeliverer.cs
Show resolved
Hide resolved
GenHub/GenHub/Features/GameProfiles/ViewModels/AddLocalContentViewModel.cs
Show resolved
Hide resolved
GenHub/GenHub/Features/GameProfiles/ViewModels/AddLocalContentViewModel.cs
Show resolved
Hide resolved
GenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileSettingsViewModel.cs
Outdated
Show resolved
Hide resolved
GenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileSettingsViewModel.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
GenHub/GenHub/Features/Content/Services/GitHub/GitHubContentDeliverer.cs (1)
141-145: Update log message to reflect expanded content types.The condition now includes
ContentType.Mod, but the log message at line 143 still only mentions "GameClient content detected."🔎 Suggested fix
- logger.LogInformation( - "GameClient content detected with {Count} ZIP files. Extracting...", - zipFiles.Count); + logger.LogInformation( + "{ContentType} content detected with {Count} ZIP files. Extracting...", + packageManifest.ContentType, + zipFiles.Count);
♻️ Duplicate comments (1)
GenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileSettingsViewModel.cs (1)
2106-2106: Fix typo in status message."unavailble" should be "unavailable".
🔎 Proposed fix
- StatusMessage = "Local content service unavailble"; + StatusMessage = "Local content service unavailable";
🧹 Nitpick comments (6)
GenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileSettingsViewModel.cs (3)
99-101: Consider explicit cleanup of messenger registration.While
WeakReferenceMessengeruses weak references, explicitly unregistering in a cleanup method (e.g., implementingIDisposableor overriding a base cleanup method) ensures deterministic resource management and prevents potential memory leaks in long-running applications.🔎 Suggested cleanup pattern
If
ViewModelBasesupports it, override or implement cleanup:protected override void OnDeactivated() { WeakReferenceMessenger.Default.Unregister<Core.Models.Content.ContentAcquiredMessage>(this); base.OnDeactivated(); }Or implement
IDisposable:public void Dispose() { WeakReferenceMessenger.Default.Unregister<Core.Models.Content.ContentAcquiredMessage>(this); }
504-512: Consider adding error logging to the message handler.The fire-and-forget pattern is acceptable here, but wrapping the call in a try-catch block within
Receivecould provide better diagnostics ifLoadAvailableContentAsyncfails during message handling.🔎 Optional improvement
public void Receive(Core.Models.Content.ContentAcquiredMessage message) { - // Refresh available content when new content is acquired - _ = LoadAvailableContentAsync(); + try + { + // Refresh available content when new content is acquired + _ = LoadAvailableContentAsync(); + } + catch (Exception ex) + { + logger?.LogError(ex, "Error refreshing content after ContentAcquiredMessage"); + } }
2123-2123: Logger null handling improved, but consider using ILoggerFactory.Passing
nullis safer than the previous cast approach, but leavesAddLocalContentViewModelwithout logging. If diagnostic logging is valuable for this dialog, injectILoggerFactoryinto this ViewModel and create a properly-typed logger.🔎 Suggested improvement using ILoggerFactory
If
ILoggerFactoryis available as a constructor parameter:- var vm = new AddLocalContentViewModel(localContentService, null); + var addContentLogger = loggerFactory?.CreateLogger<AddLocalContentViewModel>(); + var vm = new AddLocalContentViewModel(localContentService, addContentLogger);GenHub/GenHub/Common/ViewModels/MainViewModel.cs (1)
95-98: Add a comment explaining why Tools tab is disallowed as the initial tab.The silent redirect from
NavigationTab.ToolstoGameProfileslacks an explanation. Future maintainers won't understand the rationale for this behavior.🔎 Proposed fix
var tab = configurationProvider.GetLastSelectedTab(); + // Tools tab requires additional initialization that isn't ready at startup, + // so redirect to GameProfiles as the default landing tab. if (tab == NavigationTab.Tools) { tab = NavigationTab.GameProfiles; }GenHub/GenHub/Features/Content/Services/GitHub/GitHubContentDeliverer.cs (1)
157-182: Consider checking cancellation token during extraction.The extraction loop processes entries without checking
cancellationToken. For large archives, this could delay cancellation response.🔎 Suggested addition
foreach (var entry in archive.Entries) { + cancellationToken.ThrowIfCancellationRequested(); + if (string.IsNullOrEmpty(entry.Name)) continue; // Skip directoriesGenHub/GenHub/Features/GameProfiles/ViewModels/AddLocalContentViewModel.cs (1)
217-228: Consider indicating when preview is truncated.The preview limits (20 directories, 50 files) prevent performance issues but give no visual feedback when content is truncated. Users importing large folders might be confused seeing fewer items than expected.
💡 Optional enhancement
Consider adding a status message or tree node indicating truncation:
private static List<FileTreeItem> BuildDirectoryTree(DirectoryInfo dir) { var items = new List<FileTreeItem>(); + var directories = dir.GetDirectories(); + var files = dir.GetFiles(); - foreach (var d in dir.GetDirectories().Take(20)) + foreach (var d in directories.Take(20)) { items.Add(new FileTreeItem { Name = d.Name, IsFile = false, FullPath = d.FullName, Children = new ObservableCollection<FileTreeItem>(BuildDirectoryTree(d)), }); } - foreach (var f in dir.GetFiles().Take(50)) + if (directories.Length > 20) + { + items.Add(new FileTreeItem { Name = $"... and {directories.Length - 20} more directories", IsFile = false, FullPath = string.Empty }); + } + + foreach (var f in files.Take(50)) { items.Add(new FileTreeItem { Name = f.Name, IsFile = true, FullPath = f.FullName }); } + + if (files.Length > 50) + { + items.Add(new FileTreeItem { Name = $"... and {files.Length - 50} more files", IsFile = true, FullPath = string.Empty }); + } return items; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/scripts/inject-token.ps1.github/workflows/ci.ymlGenHub/GenHub/Common/ViewModels/MainViewModel.csGenHub/GenHub/Features/Content/Services/GitHub/GitHubContentDeliverer.csGenHub/GenHub/Features/GameProfiles/ViewModels/AddLocalContentViewModel.csGenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileSettingsViewModel.cs
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yml
🧰 Additional context used
🧬 Code graph analysis (1)
GenHub/GenHub/Features/GameProfiles/ViewModels/AddLocalContentViewModel.cs (2)
GenHub/GenHub/Features/GameProfiles/ViewModels/FileTreeItem.cs (1)
FileTreeItem(8-29)GenHub/GenHub/Infrastructure/Extensions/LoggerExtensions.cs (1)
LogError(46-51)
⏰ 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 (11)
GenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileSettingsViewModel.cs (2)
32-32: LGTM! Message-driven architecture implemented correctly.The
IRecipient<ContentAcquiredMessage>interface enables automatic refresh of available content when new content is acquired, following MVVM Community Toolkit patterns.
2131-2149: LGTM! Local content workflow implemented correctly.The dialog result handling properly adds the new content to the available list, enables it with the loading guard bypassed (appropriate for user-initiated actions), and provides user feedback via notification.
.github/scripts/inject-token.ps1 (1)
6-13: Approve the fork PR handling approach.The logic correctly handles missing tokens in fork PRs by using a dummy token while failing non-PR builds appropriately. The
'pull_request'event check is the correct condition since fork PRs don't have access to secrets.The application code handles the dummy token gracefully. In
UploadThingService.cs, if the token deobfuscates but the UploadThing API rejects it as invalid, the service logs an error and returnsnullrather than throwing an exception. All callers check for null and handle it appropriately, allowing the build to pass without uploads.GenHub/GenHub/Common/ViewModels/MainViewModel.cs (5)
45-58: Primary constructor with dependency injection looks good.The use of C# 12 primary constructor syntax with 14 dependencies is acceptable for a main shell ViewModel that orchestrates multiple feature view models and services. The nullable logger with default null is a reasonable pattern for optional logging.
65-88: Property initialization follows CommunityToolkit.Mvvm patterns correctly.The
[ObservableProperty]attribute and initialization from primary constructor parameters is idiomatic for the MVVM Community Toolkit.
166-175: Initialization flow with optional logging is correct.The null-conditional logger usage and background update check pattern are appropriate.
190-254: Update checking logic with proper service injection and error handling.The method correctly uses injected services, propagates cancellation tokens, dispatches UI notifications on the UI thread, and handles exceptions appropriately.
256-270: Background update check wrapper handles exceptions correctly.Separating
OperationCanceledExceptionfrom other exceptions is the right approach for cancellable background operations.GenHub/GenHub/Features/Content/Services/GitHub/GitHubContentDeliverer.cs (1)
315-329: LGTM!The storage progress adapter cleanly maps
ContentStorageProgresstoContentAcquisitionProgresswith appropriate null handling forCurrentFileName.GenHub/GenHub/Features/GameProfiles/ViewModels/AddLocalContentViewModel.cs (2)
345-345: Previous issue resolved:targetGamenow correctly usesSelectedGameType.The hardcoded fallback to
GameType.Generalsmentioned in the previous review has been fixed. The code now properly respects the user's game selection for all content types.
425-427: Previous issue resolved: Null check added forparentDir.The null-forgiving operator issue from the previous review has been fixed. Line 426 now includes an explicit null check with early continuation, preventing potential
NullReferenceExceptionwhenPath.GetDirectoryNamereturns null.
| userSettingsService.Update(settings => | ||
| { | ||
| settings.LastSelectedTab = selectedTab; | ||
| }); | ||
|
|
||
| _ = _userSettingsService.SaveAsync(); | ||
| _logger?.LogDebug("Updated last selected tab to: {Tab}", selectedTab); | ||
| _ = userSettingsService.SaveAsync(); | ||
| logger?.LogDebug("Updated last selected tab to: {Tab}", selectedTab); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger?.LogError(ex, "Failed to update selected tab setting"); | ||
| logger?.LogError(ex, "Failed to update selected tab setting"); | ||
| } |
There was a problem hiding this comment.
Fire-and-forget async may silently lose exceptions.
The _ = userSettingsService.SaveAsync() discards the task without observing it. If SaveAsync throws after the synchronous portion completes, the exception won't be caught by the surrounding try-catch block, potentially losing the user's tab preference without any indication.
🔎 Proposed fix: Observe the task with a continuation
- _ = userSettingsService.SaveAsync();
+ _ = userSettingsService.SaveAsync().ContinueWith(
+ t => logger?.LogError(t.Exception, "Failed to save user settings"),
+ TaskContinuationOptions.OnlyOnFaulted);
logger?.LogDebug("Updated last selected tab to: {Tab}", selectedTab);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| userSettingsService.Update(settings => | |
| { | |
| settings.LastSelectedTab = selectedTab; | |
| }); | |
| _ = _userSettingsService.SaveAsync(); | |
| _logger?.LogDebug("Updated last selected tab to: {Tab}", selectedTab); | |
| _ = userSettingsService.SaveAsync(); | |
| logger?.LogDebug("Updated last selected tab to: {Tab}", selectedTab); | |
| } | |
| catch (Exception ex) | |
| { | |
| _logger?.LogError(ex, "Failed to update selected tab setting"); | |
| logger?.LogError(ex, "Failed to update selected tab setting"); | |
| } | |
| userSettingsService.Update(settings => | |
| { | |
| settings.LastSelectedTab = selectedTab; | |
| }); | |
| _ = userSettingsService.SaveAsync().ContinueWith( | |
| t => logger?.LogError(t.Exception, "Failed to save user settings"), | |
| TaskContinuationOptions.OnlyOnFaulted); | |
| logger?.LogDebug("Updated last selected tab to: {Tab}", selectedTab); | |
| } | |
| catch (Exception ex) | |
| { | |
| logger?.LogError(ex, "Failed to update selected tab setting"); | |
| } |
🤖 Prompt for AI Agents
In GenHub/GenHub/Common/ViewModels/MainViewModel.cs around lines 276-287 the
call `_ = userSettingsService.SaveAsync()` is fire-and-forget which can silently
lose exceptions; either await the Task (make the calling method async and await
userSettingsService.SaveAsync()) so exceptions propagate into the existing
try/catch, or if you cannot make the method async capture the Task and attach a
continuation that logs failures (e.g.,
userSettingsService.SaveAsync().ContinueWith(t => { if (t.IsFaulted)
logger?.LogError(t.Exception, "Failed to save user settings"); },
TaskContinuationOptions.ExecuteSynchronously)); ensure exceptions from SaveAsync
are observed and logged.
| // Create progress adapter for download progress | ||
| IProgress<DownloadProgress>? downloadProgress = null; | ||
| if (progress != null) | ||
| { | ||
| downloadProgress = new Progress<DownloadProgress>(dp => | ||
| { | ||
| // Map download progress (0-100) to the Downloading phase range (40-65%) | ||
| // We start at 40 (ProgressStepDownloading) and use 25% of the range for downloads | ||
| double downloadRange = 25.0; // 40% to 65% | ||
| double fileProgressRange = downloadRange / totalFiles; | ||
| double baseProgress = ContentConstants.ProgressStepDownloading + ((currentFileIndex - 1) * fileProgressRange); | ||
| double currentProgress = baseProgress + (dp.Percentage / 100.0 * fileProgressRange); | ||
|
|
||
| progress.Report(new ContentAcquisitionProgress | ||
| { | ||
| Phase = ContentAcquisitionPhase.Downloading, | ||
| ProgressPercentage = currentProgress, | ||
| CurrentOperation = $"{file.RelativePath} ({currentFileIndex}/{totalFiles}) - {dp.Percentage:F0}% ({dp.FormattedSpeed})", | ||
| FilesProcessed = currentFileIndex - 1, | ||
| TotalFiles = totalFiles, | ||
| TotalBytes = dp.TotalBytes, | ||
| BytesProcessed = dp.BytesReceived, | ||
| CurrentFile = file.RelativePath, | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Captured loop variable in closure may cause incorrect progress reporting.
The currentFileIndex variable is captured by reference in the lambda (line 102). Since Progress<T> posts callbacks asynchronously, the callback may execute after currentFileIndex has been incremented in the next loop iteration, causing incorrect file index reporting.
🔎 Suggested fix: capture the value explicitly
// Create progress adapter for download progress
IProgress<DownloadProgress>? downloadProgress = null;
if (progress != null)
{
+ int capturedFileIndex = currentFileIndex;
downloadProgress = new Progress<DownloadProgress>(dp =>
{
// Map download progress (0-100) to the Downloading phase range (40-65%)
// We start at 40 (ProgressStepDownloading) and use 25% of the range for downloads
double downloadRange = 25.0; // 40% to 65%
double fileProgressRange = downloadRange / totalFiles;
- double baseProgress = ContentConstants.ProgressStepDownloading + ((currentFileIndex - 1) * fileProgressRange);
+ double baseProgress = ContentConstants.ProgressStepDownloading + ((capturedFileIndex - 1) * fileProgressRange);
double currentProgress = baseProgress + (dp.Percentage / 100.0 * fileProgressRange);
progress.Report(new ContentAcquisitionProgress
{
Phase = ContentAcquisitionPhase.Downloading,
ProgressPercentage = currentProgress,
- CurrentOperation = $"{file.RelativePath} ({currentFileIndex}/{totalFiles}) - {dp.Percentage:F0}% ({dp.FormattedSpeed})",
- FilesProcessed = currentFileIndex - 1,
+ CurrentOperation = $"{file.RelativePath} ({capturedFileIndex}/{totalFiles}) - {dp.Percentage:F0}% ({dp.FormattedSpeed})",
+ FilesProcessed = capturedFileIndex - 1,
TotalFiles = totalFiles,
TotalBytes = dp.TotalBytes,
BytesProcessed = dp.BytesReceived,
CurrentFile = file.RelativePath,
});
});
}🤖 Prompt for AI Agents
In GenHub/GenHub/Features/Content/Services/GitHub/GitHubContentDeliverer.cs
around lines 98 to 123, the lambda passed to new Progress<DownloadProgress>
closes over loop variables (currentFileIndex and file) so the async callback can
observe mutated values; fix this by capturing the needed values into local,
immutable variables immediately before creating the Progress (e.g., var
capturedIndex = currentFileIndex; var capturedFilePath = file.RelativePath) and
use those locals inside the lambda so each callback reports the correct file
index and path.
| foreach (var entry in archive.Entries) | ||
| { | ||
| if (string.IsNullOrEmpty(entry.Name)) continue; // Skip directories | ||
|
|
||
| var destinationPath = Path.Combine(targetDirectory, entry.FullName); | ||
| var destinationDir = Path.GetDirectoryName(destinationPath); | ||
| if (!string.IsNullOrEmpty(destinationDir)) Directory.CreateDirectory(destinationDir); | ||
|
|
||
| entry.ExtractToFile(destinationPath, overwrite: true); |
There was a problem hiding this comment.
Zip Slip vulnerability: validate extracted file paths stay within target directory.
entry.FullName from a ZIP archive can contain path traversal sequences like ../ that could write files outside the intended targetDirectory. This is a known attack vector (Zip Slip).
🔎 Suggested fix: validate the destination path
foreach (var entry in archive.Entries)
{
if (string.IsNullOrEmpty(entry.Name)) continue; // Skip directories
var destinationPath = Path.Combine(targetDirectory, entry.FullName);
+ var fullDestinationPath = Path.GetFullPath(destinationPath);
+ var fullTargetDirectory = Path.GetFullPath(targetDirectory + Path.DirectorySeparatorChar);
+
+ if (!fullDestinationPath.StartsWith(fullTargetDirectory, StringComparison.OrdinalIgnoreCase))
+ {
+ logger.LogWarning("Skipping potentially malicious ZIP entry: {EntryName}", entry.FullName);
+ continue;
+ }
+
var destinationDir = Path.GetDirectoryName(destinationPath);
if (!string.IsNullOrEmpty(destinationDir)) Directory.CreateDirectory(destinationDir);
entry.ExtractToFile(destinationPath, overwrite: true);🤖 Prompt for AI Agents
In GenHub/GenHub/Features/Content/Services/GitHub/GitHubContentDeliverer.cs
around lines 157-165, the code directly uses entry.FullName to build
destinationPath which can allow path traversal (Zip Slip); resolve the
destination path with Path.GetFullPath(Path.Combine(targetDirectory,
entry.FullName)) and compare it to the normalized targetDirectory full path
(ensure targetDirFullPath ends with a directory separator and check
destinationFullPath.StartsWith(targetDirFullPath,
StringComparison.OrdinalIgnoreCase)); if the check fails, skip the entry or
throw an exception; otherwise create the destination directory from the
validated destinationFullPath and call ExtractToFile safely with overwrite:
true.
| if (File.Exists(path)) | ||
| { | ||
| var extension = Path.GetExtension(path); | ||
| if (extension.Equals(".zip", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| await Task.Run(() => ZipFile.ExtractToDirectory(path, _stagingPath, true)); | ||
| } | ||
| else | ||
| { | ||
| var destFile = Path.Combine(_stagingPath, Path.GetFileName(path)); | ||
| File.Copy(path, destFile, true); | ||
| } | ||
| } | ||
| else if (Directory.Exists(path)) | ||
| { | ||
| var dirInfo = new DirectoryInfo(path); | ||
| var dirName = dirInfo.Name; | ||
|
|
||
| // Ensure we don't try to copy to the staging root itself if Name is somehow empty | ||
| if (string.IsNullOrWhiteSpace(dirName)) | ||
| { | ||
| dirName = "Imported_Folder"; | ||
| } | ||
|
|
||
| var targetSubDir = Path.Combine(_stagingPath, dirName); | ||
| logger?.LogDebug("ImportContentAsync: Preserving directory structure. Source: {Source}, Target: {Target}", path, targetSubDir); | ||
|
|
||
| await Task.Run(() => CopyDirectory(dirInfo, new DirectoryInfo(targetSubDir))); | ||
| } | ||
|
|
||
| // Auto-organization: If we have .map files at the root level, move them into subdirectories | ||
| // This is required because the game expects maps to be in their own folders | ||
| CreateMapFoldersIfNeeded(); |
There was a problem hiding this comment.
Wrap remaining blocking I/O operations in Task.Run.
While ZIP extraction (line 169) and directory copying (line 191) are correctly offloaded to the thread pool, File.Copy at line 174 and the CreateMapFoldersIfNeeded call at line 196 remain blocking. For large files or directories with many .map files, these synchronous operations could freeze the UI.
🔎 Proposed fix
if (File.Exists(path))
{
var extension = Path.GetExtension(path);
if (extension.Equals(".zip", StringComparison.OrdinalIgnoreCase))
{
await Task.Run(() => ZipFile.ExtractToDirectory(path, _stagingPath, true));
}
else
{
- var destFile = Path.Combine(_stagingPath, Path.GetFileName(path));
- File.Copy(path, destFile, true);
+ await Task.Run(() =>
+ {
+ var destFile = Path.Combine(_stagingPath, Path.GetFileName(path));
+ File.Copy(path, destFile, true);
+ });
}
}
else if (Directory.Exists(path))
{
var dirInfo = new DirectoryInfo(path);
var dirName = dirInfo.Name;
// Ensure we don't try to copy to the staging root itself if Name is somehow empty
if (string.IsNullOrWhiteSpace(dirName))
{
dirName = "Imported_Folder";
}
var targetSubDir = Path.Combine(_stagingPath, dirName);
logger?.LogDebug("ImportContentAsync: Preserving directory structure. Source: {Source}, Target: {Target}", path, targetSubDir);
await Task.Run(() => CopyDirectory(dirInfo, new DirectoryInfo(targetSubDir)));
}
// Auto-organization: If we have .map files at the root level, move them into subdirectories
// This is required because the game expects maps to be in their own folders
- CreateMapFoldersIfNeeded();
+ await Task.Run(CreateMapFoldersIfNeeded);🤖 Prompt for AI Agents
In GenHub/GenHub/Features/GameProfiles/ViewModels/AddLocalContentViewModel.cs
around lines 164 to 196, the File.Copy call and the CreateMapFoldersIfNeeded
invocation are synchronous and can block the UI for large operations; wrap the
File.Copy operation in Task.Run and await it (e.g. await Task.Run(() =>
File.Copy(...))) and likewise run CreateMapFoldersIfNeeded on the thread pool
(await Task.Run(() => CreateMapFoldersIfNeeded())) or make
CreateMapFoldersIfNeeded async and await its async implementation so both file
I/O and map-folder creation are executed off the UI thread.
| try | ||
| { | ||
| IsBusy = true; | ||
| StatusMessage = $"Removing {item.Name}..."; | ||
| logger?.LogInformation("Deleting item from staging: {Name} ({Path})", item.Name, item.FullPath); | ||
|
|
||
| if (item.IsFile && File.Exists(item.FullPath)) | ||
| { | ||
| File.Delete(item.FullPath); | ||
| } | ||
| else if (!item.IsFile && Directory.Exists(item.FullPath)) | ||
| { | ||
| Directory.Delete(item.FullPath, true); | ||
| } | ||
|
|
||
| await RefreshStagingTreeAsync(); | ||
| StatusMessage = $"Removed {item.Name}."; | ||
| logger?.LogInformation("Item successfully deleted: {Name}", item.Name); | ||
| Validate(); |
There was a problem hiding this comment.
Offload file and directory deletion to background thread.
File.Delete (line 295) and Directory.Delete with recursive: true (line 299) are blocking operations. Deleting large files or deep directory trees can freeze the UI.
🔎 Proposed fix
try
{
IsBusy = true;
StatusMessage = $"Removing {item.Name}...";
logger?.LogInformation("Deleting item from staging: {Name} ({Path})", item.Name, item.FullPath);
- if (item.IsFile && File.Exists(item.FullPath))
- {
- File.Delete(item.FullPath);
- }
- else if (!item.IsFile && Directory.Exists(item.FullPath))
- {
- Directory.Delete(item.FullPath, true);
- }
+ await Task.Run(() =>
+ {
+ if (item.IsFile && File.Exists(item.FullPath))
+ {
+ File.Delete(item.FullPath);
+ }
+ else if (!item.IsFile && Directory.Exists(item.FullPath))
+ {
+ Directory.Delete(item.FullPath, true);
+ }
+ });
await RefreshStagingTreeAsync();
StatusMessage = $"Removed {item.Name}.";
logger?.LogInformation("Item successfully deleted: {Name}", item.Name);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try | |
| { | |
| IsBusy = true; | |
| StatusMessage = $"Removing {item.Name}..."; | |
| logger?.LogInformation("Deleting item from staging: {Name} ({Path})", item.Name, item.FullPath); | |
| if (item.IsFile && File.Exists(item.FullPath)) | |
| { | |
| File.Delete(item.FullPath); | |
| } | |
| else if (!item.IsFile && Directory.Exists(item.FullPath)) | |
| { | |
| Directory.Delete(item.FullPath, true); | |
| } | |
| await RefreshStagingTreeAsync(); | |
| StatusMessage = $"Removed {item.Name}."; | |
| logger?.LogInformation("Item successfully deleted: {Name}", item.Name); | |
| Validate(); | |
| try | |
| { | |
| IsBusy = true; | |
| StatusMessage = $"Removing {item.Name}..."; | |
| logger?.LogInformation("Deleting item from staging: {Name} ({Path})", item.Name, item.FullPath); | |
| await Task.Run(() => | |
| { | |
| if (item.IsFile && File.Exists(item.FullPath)) | |
| { | |
| File.Delete(item.FullPath); | |
| } | |
| else if (!item.IsFile && Directory.Exists(item.FullPath)) | |
| { | |
| Directory.Delete(item.FullPath, true); | |
| } | |
| }); | |
| await RefreshStagingTreeAsync(); | |
| StatusMessage = $"Removed {item.Name}."; | |
| logger?.LogInformation("Item successfully deleted: {Name}", item.Name); | |
| Validate(); |
🤖 Prompt for AI Agents
In GenHub/GenHub/Features/GameProfiles/ViewModels/AddLocalContentViewModel.cs
around lines 287-305, the synchronous File.Delete and Directory.Delete calls
block the UI; move the deletion work onto a background thread (e.g., wrap the
file/directory delete logic in Task.Run and await it) so the UI thread stays
responsive, preserve logging and exception handling inside that background task,
and ensure IsBusy/StatusMessage are updated on the UI thread before and after
awaiting the Task.Run and that RefreshStagingTreeAsync is awaited after the
delete completes.
undead2146
force-pushed
the
feat/map-manager
branch
from
7542426 to
f259012
Compare
There was a problem hiding this comment.
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 (1)
GenHub/GenHub/Common/ViewModels/MainViewModel.cs (1)
277-295: Fire-and-forget async may silently lose exceptions.Similar to the issue at line 268, the fire-and-forget call to
DownloadsViewModel.OnTabActivatedAsync()at line 291 discards the task without observing it. If the async method throws after its synchronous portion completes, the exception will be unobserved.🔎 Proposed fix: Observe the task with error logging
else if (value == NavigationTab.Downloads) { - _ = DownloadsViewModel.OnTabActivatedAsync(); + _ = DownloadsViewModel.OnTabActivatedAsync().ContinueWith( + t => logger?.LogError(t.Exception, "Failed to activate Downloads tab"), + TaskContinuationOptions.OnlyOnFaulted); }
♻️ Duplicate comments (3)
GenHub/GenHub/Features/Content/Services/GitHub/GitHubContentDeliverer.cs (2)
98-123: Loop variable capture issue persists.The issue flagged in the previous review regarding
currentFileIndexandfilebeing captured by reference in the lambda remains unresolved. SinceProgress<T>callbacks execute asynchronously, the callback may observe mutated values after the loop has advanced.
157-165: Zip Slip vulnerability remains unaddressed.The security issue flagged in the previous review is still present. Line 161 uses
entry.FullNamedirectly without validating that the resolved path stays withintargetDirectory, allowing potential path traversal attacks.GenHub/GenHub/Common/ViewModels/MainViewModel.cs (1)
259-275: Fire-and-forget async at line 268 was previously flagged.The fire-and-forget pattern
_ = userSettingsService.SaveAsync()at line 268 remains unaddressed from previous review. As noted before, exceptions thrown after the synchronous portion completes won't be caught by the surrounding try-catch block.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/scripts/inject-token.ps1.github/workflows/ci.ymlGenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameProfiles/ViewModels/MainViewModelTests.csGenHub/GenHub/Common/ViewModels/MainViewModel.csGenHub/GenHub/Features/Content/Services/GitHub/GitHubContentDeliverer.csGenHub/GenHub/Features/GameProfiles/ViewModels/AddLocalContentViewModel.csGenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileSettingsViewModel.cs
💤 Files with no reviewable changes (2)
- GenHub/GenHub.Tests/GenHub.Tests.Core/Features/GameProfiles/ViewModels/MainViewModelTests.cs
- .github/workflows/ci.yml
✅ Files skipped from review due to trivial changes (1)
- GenHub/GenHub/Features/GameProfiles/ViewModels/AddLocalContentViewModel.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/scripts/inject-token.ps1
🧰 Additional context used
🧬 Code graph analysis (1)
GenHub/GenHub/Features/Content/Services/GitHub/GitHubContentDeliverer.cs (5)
GenHub/GenHub.Core/Models/Common/DownloadProgress.cs (1)
DownloadProgress(14-54)GenHub/GenHub.Core/Constants/ContentConstants.cs (1)
ContentConstants(6-92)GenHub/GenHub.Core/Models/Content/ContentAcquisitionProgress.cs (1)
ContentAcquisitionProgress(7-53)GenHub/GenHub/Features/Content/Services/Helpers/GitHubInferenceHelper.cs (1)
ContentType(23-41)GenHub/GenHub/Features/Content/Services/GitHub/GitHubResolver.cs (1)
ContentType(280-283)
⏰ 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 (10)
GenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileSettingsViewModel.cs (4)
32-32: LGTM: Message recipient implementation.The interface implementation follows the CommunityToolkit.Mvvm messaging pattern correctly and enables automatic content refresh when new content is acquired.
99-101: LGTM: Proper message registration.The registration uses
WeakReferenceMessenger, which prevents memory leaks by allowing the ViewModel to be garbage collected even without explicit unregistration.
504-512: LGTM: Message handler implementation.The
Receivemethod correctly implements the message handling pattern. The fire-and-forget invocation ofLoadAvailableContentAsync()is appropriate here since the method has its own error handling and theIsLoadingContentguard prevents concurrent execution issues.
2096-2157: LGTM: Past review issues resolved.The method correctly addresses previous review comments:
- Typo "unavailble" → "unavailable" (Line 2106)
- Unsafe logger cast →
nullparameter (Line 2123)The dialog flow is well-implemented with proper owner resolution, result handling, and user feedback via notifications.
GenHub/GenHub/Features/Content/Services/GitHub/GitHubContentDeliverer.cs (3)
141-141: Good enhancement to include Mod content type.Extending ZIP extraction to handle
ContentType.Modin addition toGameClientis appropriate, as mods often come packaged as ZIP archives.
168-181: Extraction progress reporting looks solid.The progress mapping from 65% to 70% with per-entry reporting provides good visibility into the extraction phase.
315-329: Storage progress integration is well-implemented.The storage progress adapter correctly maps
ContentStorageProgresstoContentAcquisitionProgresswith appropriate phase and percentage ranges (90-100%). The progress parameter is properly threaded through toAddManifestAsync.GenHub/GenHub/Common/ViewModels/MainViewModel.cs (3)
22-72: Clean constructor injection pattern.The transition to constructor-based dependency injection with public read-only properties is well-structured and follows best practices for testability and maintainability.
153-162: Background update check is appropriately fire-and-forget.The async initialization and background update check are correctly structured with cancellation token support. The fire-and-forget pattern at line 161 is acceptable for background work since exceptions are handled in the wrapper method.
177-241: Update check implementation is solid.The refactored update check logic correctly uses injected dependencies, properly handles cancellation, marshals UI updates to the correct thread, and includes comprehensive error handling.
…age and multi-instance debugging support (community-outpost#239) * Added persistent storage for manual game installations and multi-instance debugging support. * Improved update reliability with HTTP retries, GitHub API caching, and enhanced error handling. * Refactored constants and implemented automated cleanup for orphaned workspace directories.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @GenHub/GenHub/Features/Tools/MapManager/Services/TgaParser.cs:
- Around line 109-113: The guard in ConvertToRgba silently returns an all-zero
buffer when bgrData is too short; change ConvertToRgba to log a warning and
return null (use signature byte[]? ConvertToRgba(...)), and update the caller in
ParseTga to handle a null result; additionally ensure DecodeRle reports actual
filled length (or returns null/throws on truncation) so ConvertToRgba can verify
that the number of filled bytes matches expectedSrcSize before proceeding — if
the filled byte count is less than expectedSrcSize, log a warning and return
null for consistency with other error paths.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
GenHub/GenHub/Features/Tools/MapManager/Services/MapImportService.csGenHub/GenHub/Features/Tools/MapManager/Services/TgaParser.cs
⏰ 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 (1)
GenHub/GenHub/Features/Tools/MapManager/Services/MapImportService.cs (1)
26-26: LGTM! Critical fix for Windows path support.The escape sequence correction from
'\'to'\\'is necessary for C# to properly represent a literal backslash character. This ensures Windows-style paths in ZIP archives are correctly split and processed at lines 245, 403, and 417.
| int expectedSrcSize = width * height * bytesPerPixel; | ||
| if (bgrData.Length < expectedSrcSize) | ||
| { | ||
| return rgba; | ||
| } |
There was a problem hiding this comment.
Add logging and consider returning null for consistency.
The defensive guard silently returns an all-zero buffer when input data is insufficient, which is inconsistent with other error paths in this file (e.g., lines 25-26, 230-231, 250-251) that log a warning and return null. This inconsistency could hide data corruption issues by producing a valid but all-black image instead of a clear failure.
Additionally, note that this guard won't catch partially-filled buffers from DecodeRle (lines 58-101), since DecodeRle always allocates a full-size array (new byte[totalPixels * bytesPerPixel] at line 61) even if the RLE stream is truncated and only partially fills it. The array length check will pass even when the buffer content is incomplete.
Suggested improvements for consistent error handling
var rgba = new byte[width * height * 4];
int expectedSrcSize = width * height * bytesPerPixel;
if (bgrData.Length < expectedSrcSize)
{
- return rgba;
+ logger.LogWarning("Insufficient BGR(A) data: expected {Expected} bytes, got {Actual}", expectedSrcSize, bgrData.Length);
+ return null;
}Then update the return type and calling code at line 300 to handle null:
private static byte[]? ConvertToRgba(byte[] bgrData, int width, int height, int bytesPerPixel, bool topToBottom)And in ParseTga:
// Convert BGR(A) to RGBA for Avalonia
var rgbaData = ConvertToRgba(pixelData, width, height, bytesPerPixel, topToBottom);
+if (rgbaData == null)
+{
+ logger.LogWarning("Failed to convert TGA pixel data: {Path}", sourcePath);
+ return null;
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @GenHub/GenHub/Features/Tools/MapManager/Services/TgaParser.cs around lines
109-113, The guard in ConvertToRgba silently returns an all-zero buffer when
bgrData is too short; change ConvertToRgba to log a warning and return null (use
signature byte[]? ConvertToRgba(...)), and update the caller in ParseTga to
handle a null result; additionally ensure DecodeRle reports actual filled length
(or returns null/throws on truncation) so ConvertToRgba can verify that the
number of filled bytes matches expectedSrcSize before proceeding — if the filled
byte count is less than expectedSrcSize, log a warning and return null for
consistency with other error paths.
a7d7b15 to
0d4b239
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI Agents
In @GenHub/GenHub/Features/Tools/MapManager/Services/MapPackService.cs:
- Line 87: The manifest ID generation using ManifestId.Create with
ManifestConstants.DefaultManifestVersion currently only replaces spaces and can
produce invalid IDs when name contains other special characters; update the code
that builds the ID (the expression that uses name.ToLowerInvariant().Replace("
", "-")) to fully sanitize the content segment to match
PublisherContentRegexPattern by: lowercasing the name, replacing any character
not matching [a-z0-9-] with a hyphen, collapsing consecutive hyphens into one,
trimming leading/trailing hyphens, and ensuring a non-empty fallback (e.g.,
"unknown") before calling ManifestId.Create so the generated ID always
validates.
In @GenHub/GenHub/Features/Tools/Services/UploadHistoryService.cs:
- Around line 62-66: The private method InitializeInternalAsync currently only
calls RunCleanupAsync but is never used; either remove this dead method or wire
it into startup: add it to the IUploadHistoryService contract and call
InitializeInternalAsync from the service constructor (or the service
registration/startup routine) so initialization runs, or simply delete the
InitializeInternalAsync method and keep its logic where RunCleanupAsync is
invoked; reference InitializeInternalAsync and RunCleanupAsync when making the
change.
In @GenHub/GenHub/Infrastructure/Imaging/TgaImageParser.cs:
- Around line 98-101: The bounds-check in ConvertToRgba currently returns a
partially filled rgbaData on truncated sourceData; change ConvertToRgba's
signature to return byte[]? and, instead of returning rgbaData on that check,
log a warning via the class logger (include path/context if available) and
return null; then update the caller ParseTgaFile to handle a null return (check
the result, log "Failed to convert TGA data to RGBA: {Path}" or similar, and
propagate null) so truncated/decompress errors are surfaced rather than
producing corrupted images.
🧹 Nitpick comments (8)
GenHub/GenHub/Infrastructure/Converters/EnumToBoolConverter.cs (2)
14-22: LGTM! Improved null-safety and case-insensitive comparison.The changes improve robustness with the dual-check approach (direct equality + string comparison) and case-insensitive string matching. The null-safe operators on line 21 are technically redundant given the early return on lines 16-19, but they serve as defensive programming and don't impact correctness.
Optional: Remove redundant null-safe operators
Since lines 16-19 already guarantee non-null values, the
?.operators on line 21 are unnecessary:- return value?.Equals(parameter) == true || value?.ToString()?.Equals(parameter?.ToString(), StringComparison.OrdinalIgnoreCase) == true; + return value.Equals(parameter) || value.ToString().Equals(parameter.ToString(), StringComparison.OrdinalIgnoreCase);However, the current approach is acceptable as defensive programming.
25-36: Use case-insensitive parsing for consistency with Convert.The switch to
Enum.TryParseis a good improvement overEnum.Parse(safer, no exceptions). However, there's an inconsistency: theConvertmethod uses case-insensitive string comparison (line 21), butConvertBackuses case-sensitive parsing by default. This asymmetry could cause binding failures when the parameter casing doesn't match the enum exactly.🔎 Proposed fix for case sensitivity consistency
- if (Enum.TryParse(targetType, parameter.ToString(), out var result)) + if (Enum.TryParse(targetType, parameter.ToString(), ignoreCase: true, out var result)) { return result; }GenHub/GenHub.Linux/GameInstallations/SteamInstallation.cs (1)
172-172: Consider returning an interface type for better encapsulation.While the current implementation works correctly, returning
List<string>exposes implementation details. Consider returningIEnumerable<string>orIReadOnlyList<string>to provide better encapsulation and flexibility for future changes. Since this is a private method, the impact is limited to this class, but maintaining the interface-based return type would align with general best practices.🔎 Suggested refactor to use interface return type
-private List<string> GetSteamLibraryPaths() +private IEnumerable<string> GetSteamLibraryPaths()GenHub/GenHub/Features/Tools/MapManager/Views/MapManagerView.axaml (1)
282-283: Consider centralizing retention period and size limit values.The footer hard-codes "14 days" and "10MB", which duplicates the
RetentionDaysconstant and also appears at Line 204. If these values change, multiple locations need updates.Consider exposing these as ViewModel properties bound from the constants to ensure consistency across the UI.
Example approach
In the ViewModel, add properties:
public string RetentionPeriodDisplay => $"{ReplayManagerConstants.RetentionDays} days"; public string MaxUploadSizeDisplay => "10MB"; // or derive from a constantThen in XAML:
-<TextBlock Grid.Row="4" Text="Files are maintained for up to 14 days or until storage is full. Maximum 10MB per map." +<TextBlock Grid.Row="4" Text="{Binding StorageNoticeText}"GenHub/GenHub/Features/Tools/MapManager/Services/MapDirectoryService.cs (2)
234-254: Consider adding macOS support.The implementation handles Windows and Linux platforms but doesn't provide support for macOS. macOS users would be unable to open directories using this feature.
Consider adding macOS support:
Suggested enhancement
public void OpenDirectory(GameType version) { var directory = GetMapDirectory(version); EnsureDirectoryExists(version); try { if (OperatingSystem.IsWindows()) { System.Diagnostics.Process.Start("explorer.exe", directory); } else if (OperatingSystem.IsLinux()) { System.Diagnostics.Process.Start("xdg-open", directory); } + else if (OperatingSystem.IsMacOS()) + { + System.Diagnostics.Process.Start("open", directory); + } } catch (Exception ex) { logger.LogError(ex, "Failed to open map directory: {Directory}", directory); } }
257-274: Consider adding macOS support.The implementation handles Windows and Linux platforms but doesn't provide support for macOS. macOS users would be unable to reveal files using this feature.
Consider adding macOS support:
Suggested enhancement
public void RevealFile(MapFile map) { try { if (OperatingSystem.IsWindows()) { System.Diagnostics.Process.Start("explorer.exe", $"/select,\"{map.FullPath}\""); } else if (OperatingSystem.IsLinux()) { System.Diagnostics.Process.Start("xdg-open", Path.GetDirectoryName(map.FullPath) ?? map.FullPath); } + else if (OperatingSystem.IsMacOS()) + { + System.Diagnostics.Process.Start("open", new[] { "-R", map.FullPath }); + } } catch (Exception ex) { logger.LogError(ex, "Failed to reveal map: {FileName}", map.FileName); } }Note: On macOS,
open -Rreveals the file in Finder (similar to Windows'/select).GenHub/GenHub/Features/Tools/ReplayManager/Services/ReplayDirectoryService.cs (2)
114-128: Consider adding macOS support.The implementation handles Windows and Linux platforms but doesn't provide support for macOS. macOS users would be unable to open directories using this feature.
Consider adding macOS support:
Suggested enhancement
public void OpenDirectory(GameType version) { var path = GetReplayDirectory(version); if (Directory.Exists(path)) { if (OperatingSystem.IsWindows()) { Process.Start("explorer.exe", path); } else if (OperatingSystem.IsLinux()) { Process.Start("xdg-open", path); } + else if (OperatingSystem.IsMacOS()) + { + Process.Start("open", path); + } } }
131-146: Consider adding macOS support.The implementation handles Windows and Linux platforms but doesn't provide support for macOS. macOS users would be unable to reveal files using this feature.
Consider adding macOS support:
Suggested enhancement
public void RevealFile(ReplayFile replay) { if (File.Exists(replay.FullPath)) { if (OperatingSystem.IsWindows()) { Process.Start("explorer.exe", $"/select,\"{replay.FullPath}\""); } else if (OperatingSystem.IsLinux()) { // Linux doesn't have a standard 'select' argument for file managers, // so we just open the directory containing the file. Process.Start("xdg-open", Path.GetDirectoryName(replay.FullPath) ?? replay.FullPath); } + else if (OperatingSystem.IsMacOS()) + { + Process.Start("open", new[] { "-R", replay.FullPath }); + } } }Note: On macOS,
open -Rreveals the file in Finder (similar to Windows'/select).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
GenHub/GenHub.Core/Constants/MapManagerConstants.csGenHub/GenHub.Core/Constants/ReplayManagerConstants.csGenHub/GenHub.Core/Constants/UiConstants.csGenHub/GenHub.Core/Interfaces/Tools/MapManager/IMapDirectoryService.csGenHub/GenHub.Core/Interfaces/Tools/MapManager/IMapPackService.csGenHub/GenHub.Core/Interfaces/Tools/ReplayManager/IReplayDirectoryService.csGenHub/GenHub.Core/Models/Tools/ToolMetadata.csGenHub/GenHub.Linux/GameInstallations/SteamInstallation.csGenHub/GenHub.Tests/GenHub.Tests.Core/Features/Validation/FileSystemValidatorTests.csGenHub/GenHub.Windows/Program.csGenHub/GenHub/Features/Tools/MapManager/Services/MapDirectoryService.csGenHub/GenHub/Features/Tools/MapManager/Services/MapPackService.csGenHub/GenHub/Features/Tools/MapManager/Services/TgaParser.csGenHub/GenHub/Features/Tools/MapManager/ViewModels/MapManagerViewModel.csGenHub/GenHub/Features/Tools/MapManager/Views/MapManagerView.axamlGenHub/GenHub/Features/Tools/ReplayManager/Services/ReplayDirectoryService.csGenHub/GenHub/Features/Tools/ReplayManager/ViewModels/ReplayManagerViewModel.csGenHub/GenHub/Features/Tools/ReplayManager/Views/ReplayManagerView.axamlGenHub/GenHub/Features/Tools/ReplayManager/Views/ReplayManagerView.axaml.csGenHub/GenHub/Features/Tools/Services/UploadHistoryService.csGenHub/GenHub/Features/Tools/ViewModels/UploadHistoryItemViewModel.csGenHub/GenHub/Features/Validation/FileSystemValidator.csGenHub/GenHub/Infrastructure/Converters/BoolToTypeConverter.csGenHub/GenHub/Infrastructure/Converters/EnumToBoolConverter.csGenHub/GenHub/Infrastructure/Imaging/TgaImageParser.cs
💤 Files with no reviewable changes (1)
- GenHub/GenHub.Windows/Program.cs
✅ Files skipped from review due to trivial changes (1)
- GenHub/GenHub/Features/Tools/ReplayManager/Views/ReplayManagerView.axaml
🚧 Files skipped from review as they are similar to previous changes (1)
- GenHub/GenHub/Features/Tools/MapManager/Services/TgaParser.cs
🧰 Additional context used
🧬 Code graph analysis (9)
GenHub/GenHub/Features/Validation/FileSystemValidator.cs (1)
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Validation/FileSystemValidatorTests.cs (2)
Task(101-102)Task(112-113)
GenHub/GenHub/Features/Tools/ReplayManager/Views/ReplayManagerView.axaml.cs (1)
GenHub/GenHub.Core/Constants/ReplayManagerConstants.cs (1)
ReplayManagerConstants(6-47)
GenHub/GenHub/Features/Tools/ViewModels/UploadHistoryItemViewModel.cs (1)
GenHub/GenHub.Core/Constants/UiConstants.cs (1)
UiConstants(6-101)
GenHub/GenHub/Features/Tools/MapManager/ViewModels/MapManagerViewModel.cs (1)
GenHub/GenHub/Features/Tools/MapManager/Services/MapDirectoryService.cs (2)
OpenDirectory(234-254)RevealFile(257-274)
GenHub/GenHub/Features/Tools/ReplayManager/ViewModels/ReplayManagerViewModel.cs (2)
GenHub/GenHub/Features/Tools/ReplayManager/Services/ReplayDirectoryService.cs (2)
OpenDirectory(114-128)RevealFile(131-146)GenHub/GenHub.Core/Models/Tools/ReplayManager/ReplayFile.cs (1)
ReplayFile(9-52)
GenHub/GenHub/Infrastructure/Converters/EnumToBoolConverter.cs (2)
GenHub/GenHub/Infrastructure/Converters/BoolToTypeConverter.cs (1)
ConvertBack(25-28)GenHub/GenHub/Infrastructure/Converters/EqualsToConverter.cs (1)
ConvertBack(25-28)
GenHub/GenHub/Infrastructure/Converters/BoolToTypeConverter.cs (1)
GenHub/GenHub.Core/Constants/MapManagerConstants.cs (1)
MapManagerConstants(6-112)
GenHub/GenHub/Features/Tools/MapManager/Services/MapPackService.cs (1)
GenHub/GenHub.Core/Constants/ManifestConstants.cs (1)
ManifestConstants(6-152)
GenHub/GenHub.Linux/GameInstallations/SteamInstallation.cs (2)
GenHub/GenHub.Core/Models/GameClients/GameClient.cs (1)
GameClient(8-107)GenHub/GenHub.Core/Constants/GameClientConstants.cs (1)
GameClientConstants(8-244)
⏰ 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 (28)
GenHub/GenHub.Core/Interfaces/Tools/MapManager/IMapPackService.cs (1)
33-33: LGTM! Documentation typo fixed.The correction from "repoter" to "reporter" improves the documentation quality.
GenHub/GenHub.Core/Models/Tools/ToolMetadata.cs (1)
19-24: LGTM!The XML documentation comments now correctly describe their respective properties. This improves code clarity and maintainability.
GenHub/GenHub/Features/Validation/FileSystemValidator.cs (1)
35-35: LGTM! Clean refactoring from static to instance method.The conversion of
ValidateDirectoriesAsyncfrom a static method to an instance method aligns well with the constructor-injected dependencies pattern used by this class. This change improves testability and follows best practices for dependency injection.GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Validation/FileSystemValidatorTests.cs (1)
101-102: LGTM! Test properly adapted to instance method.The test correctly adapts to the instance-based
ValidateDirectoriesAsyncby callingbase.ValidateDirectoriesAsyncinstead of the previously static method. The use of thenewkeyword to expose the protected base method for testing is appropriate.GenHub/GenHub/Features/Tools/ViewModels/UploadHistoryItemViewModel.cs (1)
3-3: LGTM! Good refactoring to centralized constants.The change from hard-coded hex color values to
UiConstants.StatusSuccessColorandUiConstants.StatusErrorColorimproves maintainability and consistency across the UI. The using directive is correctly added to support these references.Also applies to: 59-59
GenHub/GenHub.Core/Constants/UiConstants.cs (1)
28-31: LGTM! Consistent addition of accent color constant.The new
AccentColorconstant is properly documented and follows the established pattern for color constants in this file. The placement before status colors is logical and maintains good organization.GenHub/GenHub.Core/Constants/MapManagerConstants.cs (1)
23-31: LGTM! Well-documented display name constants.The new constants are clearly documented and appropriately scoped. This centralizes display strings for better maintainability and potential future localization.
GenHub/GenHub/Infrastructure/Converters/BoolToTypeConverter.cs (1)
4-4: Excellent refactoring to eliminate magic strings.Replacing hardcoded display strings with centralized constants improves maintainability and provides a single source of truth for these UI labels.
Also applies to: 18-18, 21-21
GenHub/GenHub.Linux/GameInstallations/SteamInstallation.cs (3)
55-55: LGTM: Modern collection expression syntax.The change from
new()to[]uses C# 12 collection expressions, making the initialization more concise while maintaining the same functionality.
130-136: LGTM: Explicit typing with collection expressions.The explicit
string[]type declaration combined with collection expression syntax improves code clarity while modernizing the syntax.
179-179: LGTM: Target-typed new expression.Using target-typed
new()reduces redundancy and aligns with modern C# conventions while maintaining the same functionality.GenHub/GenHub/Features/Tools/MapManager/Services/MapPackService.cs (2)
44-48: LGTM: Improved variable naming.The variable rename from
subManagertosubDirectoryimproves code clarity and accurately reflects the variable's purpose.
90-90: LGTM: Modern collection expression syntax.The change from
.ToList()to the collection expression[.. mapFilePaths]is a nice modernization using C# 12 syntax while preserving the same functionality.GenHub/GenHub.Core/Constants/ReplayManagerConstants.cs (1)
32-46: LGTM! Well-documented constants centralize magic values.The new constants (ReplayExtension, ZipExtension, RetentionDays) are appropriately named, documented, and follow existing conventions in the file. This centralization improves maintainability.
GenHub/GenHub/Features/Tools/ReplayManager/Views/ReplayManagerView.axaml.cs (3)
6-6: LGTM! Good addition to support constant usage.The new using statement enables access to centralized constants, supporting the refactoring below.
59-61: LGTM! Replaces hard-coded extension with constant.Using
ReplayManagerConstants.ReplayExtensioninstead of the literal ".rep" improves maintainability and consistency.
99-100: LGTM! Centralizes file extension checks.Replacing hard-coded ".rep" and ".zip" literals with
ReplayManagerConstants.ReplayExtensionandReplayManagerConstants.ZipExtensioneliminates magic strings and ensures consistency across the codebase.GenHub/GenHub.Core/Interfaces/Tools/ReplayManager/IReplayDirectoryService.cs (1)
44-47: Method renames complete and consistent with MapManager interface.The renames from
OpenInExplorer/RevealInExplorertoOpenDirectory/RevealFilesuccessfully align withIMapDirectoryServicechanges and create platform-agnostic terminology across the codebase. Implementation inReplayDirectoryService(lines 114, 131) is complete, and all call sites have been properly updated—no remaining references to old method names exist in the codebase.GenHub/GenHub.Core/Interfaces/Tools/MapManager/IMapDirectoryService.cs (1)
44-47: Method renames successfully improve cross-platform terminology.The renames from
OpenInExplorer/RevealInExplorertoOpenDirectory/RevealFilemake the interface more platform-agnostic. The updated XML documentation correctly references "system file manager" instead of Windows-specific terminology. All implementations and call sites have been properly updated to use the new method names.GenHub/GenHub/Features/Tools/ReplayManager/ViewModels/ReplayManagerViewModel.cs (4)
8-8: LGTM: Constants namespace added.Good practice to centralize constants for maintainability.
534-535: Good refactoring: Hard-coded value replaced with constant.Replacing the hard-coded
10 * 1024 * 1024withReplayManagerConstants.MaxUploadBytesPerPeriodimproves maintainability and ensures consistency across the codebase.
608-608: LGTM: Method rename reflects interface modernization.The rename from
OpenInExplorertoOpenDirectoryaligns with the updated interface.
614-614: LGTM: Method rename reflects interface modernization.The rename from
RevealInExplorertoRevealFilealigns with the updated interface.GenHub/GenHub/Features/Tools/MapManager/ViewModels/MapManagerViewModel.cs (2)
655-655: LGTM: Method rename reflects interface modernization.The rename from
OpenInExplorertoOpenDirectoryaligns with the updated interface.
661-661: LGTM: Method rename reflects interface modernization.The rename from
RevealInExplorertoRevealFilealigns with the updated interface.GenHub/GenHub/Features/Tools/MapManager/Services/MapDirectoryService.cs (1)
130-130: LGTM: Modern collection expression syntax.The use of
[]for empty arrays is valid modern C# syntax and is more concise thannew string[0]orArray.Empty<string>().Also applies to: 162-162
GenHub/GenHub/Features/Tools/Services/UploadHistoryService.cs (2)
223-239: Good thread-safe pattern for async cleanup.The implementation correctly takes a snapshot of pending deletions under lock and releases it before performing async I/O operations. This prevents deadlocks and ensures good performance.
247-247: No changes needed — C# 12 collection expressions with target-type inference are correctly applied.Collection expressions like
[]and[.. _cache]rely on target-type inference in C# 12. Since_cacheis declared asList<UploadRecord>?andLoadHistoryInternal()returnsList<UploadRecord>, the compiler correctly infersList<UploadRecord>for these expressions, not arrays. This is idiomatic C# 12 and type-safe. The original code pattern is correct.
| var mapPack = new MapPack | ||
| { | ||
| Id = ManifestId.Create($"1.0.local.mappack.{name.ToLowerInvariant().Replace(" ", "-")}"), | ||
| Id = ManifestId.Create($"{ManifestConstants.DefaultManifestVersion}.0.local.mappack.{name.ToLowerInvariant().Replace(" ", "-")}"), |
There was a problem hiding this comment.
Incomplete input sanitization for manifest ID.
While using ManifestConstants.DefaultManifestVersion is good for consistency, the ID generation only sanitizes spaces but doesn't handle other special characters. Based on the PublisherContentRegexPattern from ManifestConstants.cs, the content name segment only allows [a-z0-9-]+. If the name parameter contains characters like parentheses, symbols, or other special characters, the generated ID will fail validation.
🔎 Proposed fix to sanitize all invalid characters
- Id = ManifestId.Create($"{ManifestConstants.DefaultManifestVersion}.0.local.mappack.{name.ToLowerInvariant().Replace(" ", "-")}"),
+ Id = ManifestId.Create($"{ManifestConstants.DefaultManifestVersion}.0.local.mappack.{System.Text.RegularExpressions.Regex.Replace(name.ToLowerInvariant(), "[^a-z0-9-]", "-").Trim('-')}"),This replaces any character that isn't a lowercase letter, number, or hyphen with a hyphen, and trims leading/trailing hyphens.
🤖 Prompt for AI Agents
In @GenHub/GenHub/Features/Tools/MapManager/Services/MapPackService.cs around
line 87, The manifest ID generation using ManifestId.Create with
ManifestConstants.DefaultManifestVersion currently only replaces spaces and can
produce invalid IDs when name contains other special characters; update the code
that builds the ID (the expression that uses name.ToLowerInvariant().Replace("
", "-")) to fully sanitize the content segment to match
PublisherContentRegexPattern by: lowercasing the name, replacing any character
not matching [a-z0-9-] with a hyphen, collapsing consecutive hyphens into one,
trimming leading/trailing hyphens, and ensuring a non-empty fallback (e.g.,
"unknown") before calling ManifestId.Create so the generated ID always
validates.
| // Initialization logic for the service | ||
| private async Task InitializeInternalAsync() | ||
| { | ||
| await RunCleanupAsync(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -nP --type=cs 'InitializeInternalAsync\s*\(' -C3Repository: undead2146/GenHub
Length of output: 660
🏁 Script executed:
rg -nP --type=cs 'class UploadHistoryService|public UploadHistoryService\s*\(' -A10 GenHub/GenHub/Features/Tools/Services/UploadHistoryService.csRepository: undead2146/GenHub
Length of output: 493
🏁 Script executed:
rg -nP --type=cs 'interface IUploadHistoryService' -A20Repository: undead2146/GenHub
Length of output: 2164
🏁 Script executed:
rg -nP --type=cs 'interface IUploadHistoryService' -A50 | head -60Repository: undead2146/GenHub
Length of output: 4779
Remove the unused InitializeInternalAsync method or add a call site.
The InitializeInternalAsync method (lines 62-66) is defined but never invoked anywhere in the codebase. It does not appear in the IUploadHistoryService interface and is not called from the constructor or any other location. If this method is intended to handle initialization after the fire-and-forget cleanup was removed, ensure it is explicitly called during service startup; otherwise, remove it as dead code.
🤖 Prompt for AI Agents
In @GenHub/GenHub/Features/Tools/Services/UploadHistoryService.cs around lines
62-66, The private method InitializeInternalAsync currently only calls
RunCleanupAsync but is never used; either remove this dead method or wire it
into startup: add it to the IUploadHistoryService contract and call
InitializeInternalAsync from the service constructor (or the service
registration/startup routine) so initialization runs, or simply delete the
InitializeInternalAsync method and keep its logic where RunCleanupAsync is
invoked; reference InitializeInternalAsync and RunCleanupAsync when making the
change.
| if (srcIndex + (sourceBytesPerPixel == 4 ? 3 : 2) >= sourceData.Length) | ||
| { | ||
| return rgbaData; | ||
| } |
There was a problem hiding this comment.
Silent failure breaks error-handling conventions and hinders debugging.
The bounds check correctly prevents an IndexOutOfRangeException, but returning partially filled rgbaData (with zeros for incomplete pixels) silently produces a corrupted image. This diverges from the established pattern in this file where errors are logged and null is returned (see lines 26-27, 207-208, 213-214, 219-220, 248-249).
When sourceData is truncated—whether from a corrupt file or a bug in DecompressRle—the caller receives no indication of failure, making debugging difficult and potentially displaying a partially black image to users.
🔎 Recommended fix: log the issue and return null
for (int i = 0; i < pixelCount; i++)
{
var srcIndex = i * sourceBytesPerPixel;
var dstIndex = i * 4;
- if (srcIndex + (sourceBytesPerPixel == 4 ? 3 : 2) >= sourceData.Length)
- {
- return rgbaData;
- }
+ if (srcIndex + (sourceBytesPerPixel == 4 ? 3 : 2) >= sourceData.Length)
+ {
+ logger.LogWarning("Insufficient source data for pixel {Pixel}/{Total}: expected {Expected} bytes, got {Actual} bytes",
+ i, pixelCount, pixelCount * sourceBytesPerPixel, sourceData.Length);
+ return null;
+ }
rgbaData[dstIndex] = sourceData[srcIndex + 2];Note: This will require updating the method signature to return byte[]? and handling the null return in ParseTgaFile at line 236:
var rgbaData = ConvertToRgba(imageData, width, height, bytesPerPixel);
if (rgbaData == null)
{
logger.LogWarning("Failed to convert TGA data to RGBA: {Path}", path);
return null;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @GenHub/GenHub/Infrastructure/Imaging/TgaImageParser.cs around lines 98-101,
The bounds-check in ConvertToRgba currently returns a partially filled rgbaData
on truncated sourceData; change ConvertToRgba's signature to return byte[]? and,
instead of returning rgbaData on that check, log a warning via the class logger
(include path/context if available) and return null; then update the caller
ParseTgaFile to handle a null return (check the result, log "Failed to convert
TGA data to RGBA: {Path}" or similar, and propagate null) so
truncated/decompress errors are surfaced rather than producing corrupted images.
0d4b239 to
296386f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
GenHub/GenHub/Features/Tools/MapManager/Views/MapManagerView.axaml (1)
204-204: Verify consistency between "per file" and "per map" terminology.Line 204 displays "(max 10MB per file)" while the footer at lines 282-283 states "Maximum 10MB per map." Ensure these messages use consistent terminology—either "per file" or "per map"—to avoid user confusion.
Also applies to: 282-283
🤖 Fix all issues with AI Agents
In
@GenHub/GenHub/Features/Tools/ReplayManager/Services/ReplayDirectoryService.cs:
- Around line 131-146: The RevealFile method currently calls Process.Start
without error handling; wrap the Windows and Linux Process.Start calls in a
try-catch, mirror the pattern used in MapDirectoryService/OpenDirectory: catch
exceptions, log the error via the same logger (use the
ReplayDirectoryService/RevealFile context), and avoid throwing so the UI won't
crash; ensure you still check File.Exists and fall back to opening the directory
when Path.GetDirectoryName returns null.
🧹 Nitpick comments (5)
GenHub/GenHub/Features/Tools/ReplayManager/ViewModels/ReplayManagerViewModel.cs (1)
537-542: Consider referencing constant value in error message.The error message on lines 538-540 displays "Maximum upload size is 10MB" as a hard-coded string. Consider dynamically referencing
ReplayManagerConstants.MaxUploadBytesPerPeriodto keep the message in sync with the actual limit:$"File too large. Maximum upload size is {ReplayManagerConstants.MaxUploadBytesPerPeriod / (1024 * 1024)}MB."This ensures the message automatically updates if the constant changes.
🔎 Proposed refactor
notificationService.ShowError( "File Too Large", - "File too large. Maximum upload size is 10MB."); + $"File too large. Maximum upload size is {ReplayManagerConstants.MaxUploadBytesPerPeriod / (1024 * 1024)}MB."); - StatusMessage = "Upload too large (Max 10MB)."; + StatusMessage = $"Upload too large (Max {ReplayManagerConstants.MaxUploadBytesPerPeriod / (1024 * 1024)}MB).";GenHub/GenHub/Features/Tools/ReplayManager/Services/ReplayDirectoryService.cs (1)
119-126: Consider adding macOS support.Both methods support Windows and Linux but lack macOS support. On macOS, you can use the
opencommand similarly to Linux'sxdg-open. ForRevealFile, useopen -Rto reveal/select the file.Example implementation
For
OpenDirectory:else if (OperatingSystem.IsMacOS()) { Process.Start("open", path); }For
RevealFile:else if (OperatingSystem.IsMacOS()) { Process.Start("open", $"-R \"{replay.FullPath}\""); }Also applies to: 135-144
GenHub/GenHub/Features/Tools/MapManager/Services/MapDirectoryService.cs (2)
234-254: Well-implemented with proper error handling.The method correctly implements cross-platform support with appropriate error handling and logging. This serves as a good pattern that should be replicated in ReplayDirectoryService.
Consider adding macOS support using
OperatingSystem.IsMacOS()and theopencommand for completeness:else if (OperatingSystem.IsMacOS()) { System.Diagnostics.Process.Start("open", directory); }
257-274: Well-implemented with proper error handling.The method correctly implements cross-platform file revealing with appropriate error handling and logging.
Consider adding macOS support for file selection using the
open -Rcommand:else if (OperatingSystem.IsMacOS()) { System.Diagnostics.Process.Start("open", $"-R \"{map.FullPath}\""); }GenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileItemViewModel.cs (1)
408-420: Property usage is correct; consider extracting version validation.The switch to using
GameVersionandPublisherproperties improves consistency. The logic correctly filters special version strings from the display.Optional: Extract duplicated version validation logic
The version string checks (lines 382-385, 408-412) are duplicated. Consider extracting to a helper method:
private static bool IsSpecialVersionString(string? version) { return string.IsNullOrEmpty(version) || version.Equals(GameClientConstants.AutoDetectedVersion, StringComparison.OrdinalIgnoreCase) || version.Equals("Unknown", StringComparison.OrdinalIgnoreCase) || version.Equals("Auto-Updated", StringComparison.OrdinalIgnoreCase) || version.Contains("Automatically", StringComparison.OrdinalIgnoreCase); }Then use:
if (IsSpecialVersionString(version))andif (!IsSpecialVersionString(GameVersion)).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
GenHub/GenHub.Core/Constants/MapManagerConstants.csGenHub/GenHub.Core/Constants/ReplayManagerConstants.csGenHub/GenHub.Core/Constants/UiConstants.csGenHub/GenHub.Core/Interfaces/Tools/MapManager/IMapDirectoryService.csGenHub/GenHub.Core/Interfaces/Tools/MapManager/IMapPackService.csGenHub/GenHub.Core/Interfaces/Tools/ReplayManager/IReplayDirectoryService.csGenHub/GenHub.Core/Models/Tools/ToolMetadata.csGenHub/GenHub.Linux/GameInstallations/SteamInstallation.csGenHub/GenHub.Tests/GenHub.Tests.Core/Features/Validation/FileSystemValidatorTests.csGenHub/GenHub.Windows/Program.csGenHub/GenHub/Features/Content/Services/GeneralsOnline/GeneralsOnlineProfileReconciler.csGenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileItemViewModel.csGenHub/GenHub/Features/Tools/MapManager/Services/MapDirectoryService.csGenHub/GenHub/Features/Tools/MapManager/Services/MapPackService.csGenHub/GenHub/Features/Tools/MapManager/Services/TgaParser.csGenHub/GenHub/Features/Tools/MapManager/ViewModels/MapManagerViewModel.csGenHub/GenHub/Features/Tools/MapManager/Views/MapManagerView.axamlGenHub/GenHub/Features/Tools/ReplayManager/Services/ReplayDirectoryService.csGenHub/GenHub/Features/Tools/ReplayManager/ViewModels/ReplayManagerViewModel.csGenHub/GenHub/Features/Tools/ReplayManager/Views/ReplayManagerView.axamlGenHub/GenHub/Features/Tools/ReplayManager/Views/ReplayManagerView.axaml.csGenHub/GenHub/Features/Tools/Services/UploadHistoryService.csGenHub/GenHub/Features/Tools/ViewModels/UploadHistoryItemViewModel.csGenHub/GenHub/Features/Validation/FileSystemValidator.csGenHub/GenHub/Infrastructure/Converters/BoolToTypeConverter.csGenHub/GenHub/Infrastructure/Converters/EnumToBoolConverter.csGenHub/GenHub/Infrastructure/Imaging/TgaImageParser.cs
💤 Files with no reviewable changes (1)
- GenHub/GenHub.Windows/Program.cs
✅ Files skipped from review due to trivial changes (2)
- GenHub/GenHub/Features/Content/Services/GeneralsOnline/GeneralsOnlineProfileReconciler.cs
- GenHub/GenHub.Core/Models/Tools/ToolMetadata.cs
🚧 Files skipped from review as they are similar to previous changes (14)
- GenHub/GenHub/Features/Tools/ViewModels/UploadHistoryItemViewModel.cs
- GenHub/GenHub/Features/Tools/Services/UploadHistoryService.cs
- GenHub/GenHub/Features/Tools/MapManager/ViewModels/MapManagerViewModel.cs
- GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Validation/FileSystemValidatorTests.cs
- GenHub/GenHub.Core/Interfaces/Tools/MapManager/IMapDirectoryService.cs
- GenHub/GenHub/Infrastructure/Converters/BoolToTypeConverter.cs
- GenHub/GenHub/Features/Tools/MapManager/Services/MapPackService.cs
- GenHub/GenHub.Core/Interfaces/Tools/MapManager/IMapPackService.cs
- GenHub/GenHub.Core/Constants/ReplayManagerConstants.cs
- GenHub/GenHub/Infrastructure/Converters/EnumToBoolConverter.cs
- GenHub/GenHub.Core/Constants/MapManagerConstants.cs
- GenHub/GenHub/Infrastructure/Imaging/TgaImageParser.cs
- GenHub/GenHub/Features/Tools/MapManager/Services/TgaParser.cs
- GenHub/GenHub.Linux/GameInstallations/SteamInstallation.cs
🧰 Additional context used
🧬 Code graph analysis (3)
GenHub/GenHub/Features/Tools/ReplayManager/ViewModels/ReplayManagerViewModel.cs (3)
GenHub/GenHub.Core/Constants/ReplayManagerConstants.cs (1)
ReplayManagerConstants(6-47)GenHub/GenHub.Core/Interfaces/Tools/ReplayManager/IReplayDirectoryService.cs (2)
OpenDirectory(47-47)RevealFile(53-53)GenHub/GenHub/Features/Tools/ReplayManager/Services/ReplayDirectoryService.cs (2)
OpenDirectory(114-128)RevealFile(131-146)
GenHub/GenHub/Features/Tools/ReplayManager/Views/ReplayManagerView.axaml.cs (1)
GenHub/GenHub.Core/Constants/ReplayManagerConstants.cs (1)
ReplayManagerConstants(6-47)
GenHub/GenHub/Features/Tools/ReplayManager/Services/ReplayDirectoryService.cs (3)
GenHub/GenHub.Core/Interfaces/Tools/MapManager/IMapDirectoryService.cs (2)
OpenDirectory(47-47)RevealFile(53-53)GenHub/GenHub.Core/Interfaces/Tools/ReplayManager/IReplayDirectoryService.cs (2)
OpenDirectory(47-47)RevealFile(53-53)GenHub/GenHub.Core/Models/Tools/ReplayManager/ReplayFile.cs (1)
ReplayFile(9-52)
⏰ 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 (9)
GenHub/GenHub/Features/Validation/FileSystemValidator.cs (1)
35-35: LGTM! Improved consistency by converting to instance method.Converting
ValidateDirectoriesAsyncfrom static to instance method aligns with the pattern used byValidateFilesAsync(line 70) and improves consistency within the class. While the method doesn't currently access instance members, this change provides flexibility to use_loggeror_hashProviderin the future without requiring breaking changes to derived classes.GenHub/GenHub.Core/Constants/UiConstants.cs (1)
28-31: LGTM! Global accent color added correctly.The
AccentColorconstant is properly implemented with a valid hex color value and clear documentation. The placement before status colors is appropriate for a global UI accent color.GenHub/GenHub/Features/Tools/ReplayManager/Views/ReplayManagerView.axaml.cs (1)
6-6: LGTM! Constants usage improves maintainability.The refactoring to use
ReplayManagerConstants.ReplayExtensionandReplayManagerConstants.ZipExtensioncentralizes file extension handling and improves consistency across the codebase.Also applies to: 59-61, 99-100
GenHub/GenHub/Features/Tools/ReplayManager/Views/ReplayManagerView.axaml (1)
183-183: LGTM! File size messaging aligns with constants.The updated text correctly reflects the size limits defined in
ReplayManagerConstants: 1MB per replay file and 10MB total per 3-day period. The messaging is clear and consistent.Also applies to: 261-262
GenHub/GenHub/Features/Tools/ReplayManager/ViewModels/ReplayManagerViewModel.cs (2)
8-8: LGTM! Constants usage improves maintainability.The refactoring to use
ReplayManagerConstants.MaxUploadBytesPerPeriodcentralizes the size limit configuration and ensures consistency across the codebase.Also applies to: 534-535
608-608: LGTM! Method renames improve cross-platform clarity.The method renames from
OpenInExplorer/RevealInExplorertoOpenDirectory/RevealFilemake the API more platform-agnostic and better reflect their behavior on non-Windows systems.Also applies to: 614-614
GenHub/GenHub.Core/Interfaces/Tools/ReplayManager/IReplayDirectoryService.cs (1)
44-47: LGTM! Interface method renames improve cross-platform clarity.The method renames from
OpenInExplorer/RevealInExplorertoOpenDirectory/RevealFilebetter reflect platform-agnostic behavior. The updated XML documentation appropriately references "system file manager" terminology.Also applies to: 50-53
GenHub/GenHub/Features/Tools/MapManager/Services/MapDirectoryService.cs (1)
86-90: No action needed. TheAssetFilesproperty is typed asList<string>(notstring[]), so line 90's.ToList()is correct and matches the property type. All assignments at lines 109, 130, and 162 are type-consistent.Likely an incorrect or invalid review comment.
GenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileItemViewModel.cs (1)
378-393: Correct usage of the generated GameVersion property.The refactoring from the private backing field
_gameVersionto the public observable propertyGameVersionis correct and follows MVVM best practices. PropertyChanged notifications fired during construction are harmless since no subscribers exist yet.
| public void OpenDirectory(GameType version) | ||
| { | ||
| var path = GetReplayDirectory(version); | ||
| if (Directory.Exists(path)) | ||
| { | ||
| Process.Start(new ProcessStartInfo | ||
| if (OperatingSystem.IsWindows()) | ||
| { | ||
| FileName = PlatformConstants.WindowsExplorerExecutable, | ||
| Arguments = path, | ||
| UseShellExecute = true, | ||
| }); | ||
| Process.Start("explorer.exe", path); | ||
| } | ||
| else if (OperatingSystem.IsLinux()) | ||
| { | ||
| Process.Start("xdg-open", path); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling to prevent unhandled exceptions.
The Process.Start calls can throw exceptions (e.g., Win32Exception, FileNotFoundException, ObjectDisposedException) that will propagate to callers. MapDirectoryService implements the same logic with proper try-catch blocks and error logging (lines 239-253). Apply the same pattern here for consistency and robustness.
🔎 Proposed fix with error handling
public void OpenDirectory(GameType version)
{
var path = GetReplayDirectory(version);
- if (Directory.Exists(path))
+ EnsureDirectoryExists(version);
+
+ try
{
if (OperatingSystem.IsWindows())
{
Process.Start("explorer.exe", path);
}
else if (OperatingSystem.IsLinux())
{
Process.Start("xdg-open", path);
}
}
+ catch (Exception ex)
+ {
+ logger.LogError(ex, "Failed to open replay directory: {Directory}", path);
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| public void RevealFile(ReplayFile replay) | ||
| { | ||
| if (File.Exists(replay.FullPath)) | ||
| { | ||
| Process.Start(new ProcessStartInfo | ||
| if (OperatingSystem.IsWindows()) | ||
| { | ||
| Process.Start("explorer.exe", $"/select,\"{replay.FullPath}\""); | ||
| } | ||
| else if (OperatingSystem.IsLinux()) | ||
| { | ||
| FileName = PlatformConstants.WindowsExplorerExecutable, | ||
| Arguments = string.Format(PlatformConstants.WindowsExplorerSelectArgument, replay.FullPath), | ||
| UseShellExecute = true, | ||
| }); | ||
| // Linux doesn't have a standard 'select' argument for file managers, | ||
| // so we just open the directory containing the file. | ||
| Process.Start("xdg-open", Path.GetDirectoryName(replay.FullPath) ?? replay.FullPath); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling to prevent unhandled exceptions.
Similar to OpenDirectory, the Process.Start calls lack error handling. MapDirectoryService implements identical logic with try-catch blocks and error logging (lines 257-274). Apply the same pattern for consistency.
🔎 Proposed fix with error handling
public void RevealFile(ReplayFile replay)
{
- if (File.Exists(replay.FullPath))
+ try
{
if (OperatingSystem.IsWindows())
{
Process.Start("explorer.exe", $"/select,\"{replay.FullPath}\"");
}
else if (OperatingSystem.IsLinux())
{
- // Linux doesn't have a standard 'select' argument for file managers,
- // so we just open the directory containing the file.
Process.Start("xdg-open", Path.GetDirectoryName(replay.FullPath) ?? replay.FullPath);
}
}
+ catch (Exception ex)
+ {
+ logger.LogError(ex, "Failed to reveal replay: {FileName}", replay.FileName);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void RevealFile(ReplayFile replay) | |
| { | |
| if (File.Exists(replay.FullPath)) | |
| { | |
| Process.Start(new ProcessStartInfo | |
| if (OperatingSystem.IsWindows()) | |
| { | |
| Process.Start("explorer.exe", $"/select,\"{replay.FullPath}\""); | |
| } | |
| else if (OperatingSystem.IsLinux()) | |
| { | |
| FileName = PlatformConstants.WindowsExplorerExecutable, | |
| Arguments = string.Format(PlatformConstants.WindowsExplorerSelectArgument, replay.FullPath), | |
| UseShellExecute = true, | |
| }); | |
| // Linux doesn't have a standard 'select' argument for file managers, | |
| // so we just open the directory containing the file. | |
| Process.Start("xdg-open", Path.GetDirectoryName(replay.FullPath) ?? replay.FullPath); | |
| } | |
| } | |
| } | |
| public void RevealFile(ReplayFile replay) | |
| { | |
| try | |
| { | |
| if (File.Exists(replay.FullPath)) | |
| { | |
| if (OperatingSystem.IsWindows()) | |
| { | |
| Process.Start("explorer.exe", $"/select,\"{replay.FullPath}\""); | |
| } | |
| else if (OperatingSystem.IsLinux()) | |
| { | |
| // Linux doesn't have a standard 'select' argument for file managers, | |
| // so we just open the directory containing the file. | |
| Process.Start("xdg-open", Path.GetDirectoryName(replay.FullPath) ?? replay.FullPath); | |
| } | |
| } | |
| } | |
| catch (Exception ex) | |
| { | |
| logger.LogError(ex, "Failed to reveal replay: {FileName}", replay.FileName); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
@GenHub/GenHub/Features/Tools/ReplayManager/Services/ReplayDirectoryService.cs
around lines 131-146, The RevealFile method currently calls Process.Start
without error handling; wrap the Windows and Linux Process.Start calls in a
try-catch, mirror the pattern used in MapDirectoryService/OpenDirectory: catch
exceptions, log the error via the same logger (use the
ReplayDirectoryService/RevealFile context), and avoid throwing so the UI won't
crash; ensure you still check File.Exists and fall back to opening the directory
when Path.GetDirectoryName returns null.
2bb290d to
90ef3d1
Compare
…port and GitHub rate limit tracking (community-outpost#237) This update introduces a persistent notification feed with history tracking and a redesigned bell UI featuring dynamic unread badges. It adds support for multi-action notifications with styled buttons and integrates a GitHub rate limit tracker to provide real-time API usage warnings. The core notification architecture has been refactored into record types for better immutability, alongside service enhancements for managing read states and history. Additionally, the UI features a new custom title bar and redesigned toasts, backed by comprehensive unit tests to ensure thread safety and reliability.
…le management - Implemented ReplayManagerView for managing replay files. - Added drag-and-drop support for importing replay files (.rep, .zip). - Enhanced ToolsViewModel to include Replay Manager services. - Updated UI styles for better visual consistency. - Introduced EnumToBoolConverter and EqualsToConverter for improved data binding. - Added documentation for Replay Manager features and usage. - Registered Replay Manager services in the dependency injection module.
undead2146
force-pushed
the
feat/map-manager
branch
from
90ef3d1 to
29bb7f1
Compare
undead2146
force-pushed
the
feat/map-manager
branch
from
29bb7f1 to
444b837
Compare
…y launching (community-outpost#241) * feat: Implement SteamLauncher for game directory preparation and proxy launching - Added SteamLauncher service to manage game directory preparation for Steam-tracked profiles. - Introduced ProxyConfig for configuring the proxy launcher with target executable and working directory. - Implemented methods for deploying the proxy launcher and creating necessary configuration files. - Enhanced manifest generation to support backup executable handling from the Steam Proxy Launcher. - Updated SteamManifestPatcher to improve executable validation logic. - Refactored HardLinkStrategy to prioritize file deduplication based on content type. - Improved error handling and logging throughout the game installation and manifest processes. - Updated documentation to reflect changes in detection and manifest creation workflows. * fix: resolve proxy launcher not replacing generals.exe * chore: resolve greptile comments * fix: resolve GameProfileSettingsWindow drag to resize from fullscreen
undead2146
force-pushed
the
feat/map-manager
branch
from
e33e8d0 to
c8788f0
Compare
Summary by CodeRabbit
New Features
Improvements
Refactor
UI Updates
✏️ Tip: You can customize this high-level summary in your review settings.