Skip to content
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions GenHub/GenHub.Core/Constants/GenLauncherConstants.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
namespace GenHub.Core.Constants;

/// <summary>
/// Constants for GenLauncher file normalization.
/// </summary>
public static class GenLauncherConstants
{
/// <summary>
/// GenLauncher Replace suffix - appended to original game files when temporarily disabled.
/// </summary>
public const string ReplaceSuffix = ".GLR";

/// <summary>
/// GenLauncher Original File suffix - backup suffix for original files before modification.
/// </summary>
public const string OriginalFileSuffix = ".GOF";

/// <summary>
/// GenLauncher Temp Copy suffix - temporary folder suffix for version copies.
/// </summary>
public const string TempCopySuffix = ".GLTC";

/// <summary>
/// GenLauncher scrambled .big file extension.
/// </summary>
public const string GibExtension = ".gib";

/// <summary>
/// Standard .big file extension.
/// </summary>
public const string BigExtension = ".big";

/// <summary>
/// All GenLauncher suffixes that should be removed during normalization.
/// </summary>
public static readonly string[] AllSuffixes =
[
ReplaceSuffix,
OriginalFileSuffix,
TempCopySuffix,
];

/// <summary>
/// Session key for "do not ask again" preference for normalization dialog.
/// </summary>
Comment thread
greptile-apps[bot] marked this conversation as resolved.
public const string NormalizationDialogSessionKey = "genlauncher.normalization.skip";

Check warning on line 46 in GenHub/GenHub.Core/Constants/GenLauncherConstants.cs

View workflow job for this annotation

GitHub Actions / Build Linux

Constant fields should appear before non-constant fields (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1203.md)

Check warning on line 46 in GenHub/GenHub.Core/Constants/GenLauncherConstants.cs

View workflow job for this annotation

GitHub Actions / Build Windows

Constant fields should appear before non-constant fields (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1203.md)

Check warning on line 46 in GenHub/GenHub.Core/Constants/GenLauncherConstants.cs

View workflow job for this annotation

GitHub Actions / Build Windows

Constant fields should appear before non-constant fields (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1203.md)

Check warning on line 46 in GenHub/GenHub.Core/Constants/GenLauncherConstants.cs

View workflow job for this annotation

GitHub Actions / Build Windows

Constant fields should appear before non-constant fields (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1203.md)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
using System.Collections.Generic;

namespace GenHub.Core.Interfaces.Content;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Result classes misplaced in Interfaces namespace

Both GenLauncherDetectionResult and GenLauncherNormalizationResult are declared under the GenHub.Core.Interfaces.Content namespace and physically live inside the GenHub.Core/Interfaces/Content/ folder. Every other file in that folder is an I* interface — the folder/namespace contract is "interfaces only." The project already has a dedicated home for result/DTO classes: GenHub.Core/Models/Results/ (which holds OperationResult.cs, OperationResultOfT.cs, ResultBase.cs). Placing concrete data classes inside the Interfaces namespace is misleading and violates the existing structural convention.

Both files should be moved to GenHub.Core/Models/Results/ (or GenHub.Core/Models/Content/) and their namespace updated accordingly:

// GenHub.Core/Models/Results/GenLauncherDetectionResult.cs
namespace GenHub.Core.Models.Results;

The same applies to GenHub.Core/Interfaces/Content/GenLauncherNormalizationResult.cs (line 3).

Prompt To Fix With AI
This is a comment left during a code review.
Path: GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs
Line: 3

Comment:
**Result classes misplaced in `Interfaces` namespace**

Both `GenLauncherDetectionResult` and `GenLauncherNormalizationResult` are declared under the `GenHub.Core.Interfaces.Content` namespace and physically live inside the `GenHub.Core/Interfaces/Content/` folder. Every other file in that folder is an `I*` interface — the folder/namespace contract is "interfaces only." The project already has a dedicated home for result/DTO classes: `GenHub.Core/Models/Results/` (which holds `OperationResult.cs`, `OperationResultOfT.cs`, `ResultBase.cs`). Placing concrete data classes inside the `Interfaces` namespace is misleading and violates the existing structural convention.

Both files should be moved to `GenHub.Core/Models/Results/` (or `GenHub.Core/Models/Content/`) and their namespace updated accordingly:

```csharp
// GenHub.Core/Models/Results/GenLauncherDetectionResult.cs
namespace GenHub.Core.Models.Results;
```

The same applies to `GenHub.Core/Interfaces/Content/GenLauncherNormalizationResult.cs` (line 3).

How can I resolve this? If you propose a fix, please make it concise.


/// <summary>
/// Result of GenLauncher file detection.
/// </summary>
public class GenLauncherDetectionResult
{
/// <summary>
/// Whether any GenLauncher files were detected.
/// </summary>
public bool HasGenLauncherFiles { get; set; }

Check warning on line 13 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Linux

The property's documentation summary text should begin with: 'Gets or sets a value indicating whether' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 13 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets or sets a value indicating whether' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 13 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets or sets a value indicating whether' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 13 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets or sets a value indicating whether' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Make HasGenLauncherFiles derived, not mutable.

This flag can drift from TotalAffectedFiles because both it and all the collections are mutable, and callers trust it to decide whether normalization runs. Computing it from the lists keeps the DTO internally consistent.

Small consistency fix
-    public bool HasGenLauncherFiles { get; set; }
+    public bool HasGenLauncherFiles => TotalAffectedFiles > 0;

Also applies to: 43-44

🧰 Tools
🪛 GitHub Check: Build Linux

[warning] 13-13:
The property's documentation summary text should begin with: 'Gets or sets a value indicating whether' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs` at line
13, Make HasGenLauncherFiles a derived, read-only property instead of a mutable
auto-property: remove the setter and implement the getter to compute its value
from the existing collections/counts (e.g., return TotalAffectedFiles > 0 or
check the relevant affected-file collections' Any()/Count > 0) so the DTO stays
consistent with the mutable lists; update the
GenLauncherDetectionResult.HasGenLauncherFiles property accordingly.


/// <summary>
/// List of .gib files found.
/// </summary>
public List<string> GibFiles { get; set; } = [];

Check warning on line 18 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Linux

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 18 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 18 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 18 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

/// <summary>
/// List of files with .GLR suffix.
/// </summary>
public List<string> GlrFiles { get; set; } = [];

Check warning on line 23 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Linux

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 23 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 23 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 23 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

/// <summary>
/// List of files with .GOF suffix.
/// </summary>
public List<string> GofFiles { get; set; } = [];

Check warning on line 28 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Linux

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 28 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 28 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 28 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

/// <summary>
/// List of files with .GLTC suffix.
/// </summary>
public List<string> GltcFiles { get; set; } = [];

Check warning on line 33 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Linux

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 33 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 33 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 33 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

/// <summary>
/// List of symbolic links detected.
/// </summary>
public List<string> SymbolicLinks { get; set; } = [];

Check warning on line 38 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Linux

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 38 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 38 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 38 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

/// <summary>
/// Total count of affected files.
/// </summary>
public int TotalAffectedFiles =>

Check warning on line 43 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Linux

The property's documentation summary text should begin with: 'Gets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 43 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 43 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 43 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherDetectionResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)
GibFiles.Count + GlrFiles.Count + GofFiles.Count + GltcFiles.Count + SymbolicLinks.Count;

/// <summary>
/// Gets a user-friendly summary of detected files.
/// </summary>
/// <returns>Summary string.</returns>
public string GetSummary()
{
var parts = new List<string>();
if (GibFiles.Count > 0)
{
parts.Add($"{GibFiles.Count} .gib file(s)");
}

if (GlrFiles.Count > 0)
{
parts.Add($"{GlrFiles.Count} .GLR file(s)");
}

if (GofFiles.Count > 0)
{
parts.Add($"{GofFiles.Count} .GOF file(s)");
}

if (GltcFiles.Count > 0)
{
parts.Add($"{GltcFiles.Count} .GLTC file(s)");
}

if (SymbolicLinks.Count > 0)
{
parts.Add($"{SymbolicLinks.Count} symbolic link(s)");
Comment thread
greptile-apps[bot] marked this conversation as resolved.
}

return parts.Count > 0 ? string.Join(", ", parts) : "No GenLauncher files detected";
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
using System.Collections.Generic;

namespace GenHub.Core.Interfaces.Content;

/// <summary>
/// Result of GenLauncher file normalization.
/// </summary>
public class GenLauncherNormalizationResult
{
/// <summary>
/// Number of files successfully normalized.
/// </summary>
public int NormalizedCount { get; set; }

Check warning on line 13 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherNormalizationResult.cs

View workflow job for this annotation

GitHub Actions / Build Linux

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 13 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherNormalizationResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 13 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherNormalizationResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 13 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherNormalizationResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

/// <summary>
/// Number of symbolic links removed.
/// </summary>
public int SymbolicLinksRemoved { get; set; }

Check warning on line 18 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherNormalizationResult.cs

View workflow job for this annotation

GitHub Actions / Build Linux

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 18 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherNormalizationResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 18 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherNormalizationResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

Check warning on line 18 in GenHub/GenHub.Core/Interfaces/Content/GenLauncherNormalizationResult.cs

View workflow job for this annotation

GitHub Actions / Build Windows

The property's documentation summary text should begin with: 'Gets or sets' (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md)

/// <summary>
/// List of files that failed to normalize.
/// </summary>
public List<string> FailedFiles { get; set; } = [];

/// <summary>
/// Whether normalization was fully successful.
/// </summary>
public bool IsFullySuccessful => FailedFiles.Count == 0;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
using System.Threading;
using System.Threading.Tasks;
using GenHub.Core.Models.Results;

namespace GenHub.Core.Interfaces.Content;

/// <summary>
/// Service for detecting and normalizing GenLauncher file modifications.
/// </summary>
public interface IGenLauncherNormalizationService
{
/// <summary>
/// Detects GenLauncher files (.gib, .GLR, .GOF, .GLTC) in the specified directory.
/// </summary>
/// <param name="directoryPath">The directory to scan.</param>
/// <returns>Detection result with list of affected files.</returns>
Task<GenLauncherDetectionResult> DetectGenLauncherFilesAsync(string directoryPath);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add cancellation to detection as well.

DetectGenLauncherFilesAsync does recursive filesystem work in the implementation, but this signature gives callers no way to stop it when the import flow is canceled. Add a CancellationToken here and thread it through the implementation and caller alongside NormalizeFilesAsync.

Minimal contract change
-    Task<GenLauncherDetectionResult> DetectGenLauncherFilesAsync(string directoryPath);
+    Task<GenLauncherDetectionResult> DetectGenLauncherFilesAsync(
+        string directoryPath,
+        CancellationToken cancellationToken = default);

Based on learnings, async methods that perform real work should accept and propagate CancellationToken when cancellation is observable.

📝 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.

Suggested change
Task<GenLauncherDetectionResult> DetectGenLauncherFilesAsync(string directoryPath);
Task<GenLauncherDetectionResult> DetectGenLauncherFilesAsync(
string directoryPath,
CancellationToken cancellationToken = default);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@GenHub/GenHub.Core/Interfaces/Content/IGenLauncherNormalizationService.cs` at
line 17, Update the IGenLauncherNormalizationService contract to accept a
CancellationToken on DetectGenLauncherFilesAsync (change signature to
DetectGenLauncherFilesAsync(string directoryPath, CancellationToken
cancellationToken)), then update the corresponding implementation(s) of
DetectGenLauncherFilesAsync to accept and respect that token (check cancellation
during recursion and pass it into any async/IO calls), and update all call sites
to pass the same CancellationToken you already thread through for
NormalizeFilesAsync so cancellation is propagated consistently.

Comment thread
greptile-apps[bot] marked this conversation as resolved.
Outdated

/// <summary>
/// Normalizes GenLauncher files in the specified directory.
/// Converts .gib to .big and removes .GLR, .GOF, .GLTC suffixes.
/// </summary>
/// <param name="directoryPath">The directory containing files to normalize.</param>
/// <param name="cancellationToken">Cancellation token.</param>
/// <returns>Operation result with normalization details.</returns>
Task<OperationResult<GenLauncherNormalizationResult>> NormalizeFilesAsync(
string directoryPath,
CancellationToken cancellationToken = default);
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
using System.Threading.Tasks;
using CommunityToolkit.Mvvm.ComponentModel;
using CommunityToolkit.Mvvm.Input;
using GenHub.Core.Constants;
using GenHub.Core.Interfaces.Common;
using GenHub.Core.Interfaces.Content;
using GenHub.Core.Models.Enums;
using Microsoft.Extensions.Logging;
Expand All @@ -19,10 +21,14 @@ namespace GenHub.Features.GameProfiles.ViewModels;
/// </summary>
/// <param name="localContentService">Service for handling local content operations.</param>
/// <param name="contentStorageService">Service for content storage operations.</param>
/// <param name="genLauncherNormalizationService">Service for GenLauncher file normalization.</param>
/// <param name="dialogService">Service for showing dialogs.</param>
/// <param name="logger">Logger instance.</param>
public partial class AddLocalContentViewModel(
ILocalContentService localContentService,
IContentStorageService? contentStorageService,
IGenLauncherNormalizationService? genLauncherNormalizationService,
IDialogService? dialogService,
ILogger<AddLocalContentViewModel>? logger = null) : ObservableObject
{
/// <summary>
Expand Down Expand Up @@ -369,8 +375,81 @@ public async Task ImportContentAsync(string path)
// Auto-organization: If we have .map files at the root level, move them into subdirectories
CreateMapFoldersIfNeeded();

// Detect and normalize GenLauncher files
try
{
if (genLauncherNormalizationService != null && dialogService != null)
{
var detectionResult = await genLauncherNormalizationService.DetectGenLauncherFilesAsync(_stagingPath);

if (detectionResult.HasGenLauncherFiles)
{
logger?.LogInformation("GenLauncher files detected: {Summary}", detectionResult.GetSummary());

var shouldNormalize = await dialogService.ShowConfirmationAsync(
"GenLauncher Files Detected",
$"This content contains GenLauncher-modified files:\n\n{detectionResult.GetSummary()}\n\nWould you like to normalize these files to standard format?\n\n" +
"This will:\n" +
"• Convert .gib files to .big\n" +
"• Remove .GLR, .GOF, .GLTC suffixes\n" +
"• Remove symbolic links",
"Normalize",
"Skip",
sessionKey: GenLauncherConstants.NormalizationDialogSessionKey);

if (shouldNormalize)
{
StatusMessage = "Normalizing GenLauncher files...";
logger?.LogInformation("User confirmed normalization");

var normalizationResult = await genLauncherNormalizationService.NormalizeFilesAsync(
_stagingPath,
CancellationToken.None);

if (normalizationResult.Success)
{
var result = normalizationResult.Data;
StatusMessage = $"Normalized {result.NormalizedCount} file(s). Import successful.";
logger?.LogInformation(
"Normalization completed: {NormalizedCount} files, {SymlinksRemoved} symlinks removed",
result.NormalizedCount,
result.SymbolicLinksRemoved);

if (!result.IsFullySuccessful)
{
logger?.LogWarning(
"Some files failed to normalize: {FailedFiles}",
string.Join(", ", result.FailedFiles));
}
Comment on lines +412 to +423

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Surface partial normalization failures in the UI.

When Success is true but IsFullySuccessful is false, the status still says "Normalized ... Import successful." and only the log mentions FailedFiles. That hides unresolved conflicts/permission errors from the person importing the content.

Minimal status fix
-                                StatusMessage = $"Normalized {result.NormalizedCount} file(s). Import successful.";
+                                StatusMessage = result.IsFullySuccessful
+                                    ? $"Normalized {result.NormalizedCount} file(s). Import successful."
+                                    : $"Import completed with warnings: normalized {result.NormalizedCount} file(s), removed {result.SymbolicLinksRemoved} symbolic link(s), but {result.FailedFiles.Count} file(s) could not be normalized.";
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@GenHub/GenHub/Features/GameProfiles/ViewModels/AddLocalContentViewModel.cs`
around lines 412 - 423, The StatusMessage currently always says "Import
successful" even when result.IsFullySuccessful is false; update the code
handling StatusMessage (around StatusMessage assignment and the if
(!result.IsFullySuccessful) block) to surface partial failures to the UI by
setting StatusMessage to reflect the partial failure—e.g., change it to
something like "Normalized X files; Y failed: [file1, file2]" when
result.IsFullySuccessful is false and include result.FailedFiles (use
string.Join) and/or result.SymbolicLinksRemoved; keep the existing
logger.LogWarning but ensure the UI text is clear about partial failures and not
just the log.

}
else
{
StatusMessage = $"Normalization warning: {normalizationResult.FirstError}. Import will continue.";
logger?.LogWarning("Normalization failed: {Error}", normalizationResult.FirstError);
}
}
else
{
logger?.LogInformation("User skipped normalization");
StatusMessage = "Import successful (GenLauncher files not normalized).";
}
}
}
}
catch (Exception ex)
{
logger?.LogError(ex, "Error during GenLauncher detection/normalization");
StatusMessage = "Import successful (normalization check failed).";
}

await RefreshStagingTreeAsync();
StatusMessage = "Import successful.";

// Only set generic message if normalization didn't set a specific one
if (!StatusMessage.Contains("Normalized") && !StatusMessage.Contains("GenLauncher"))
{
StatusMessage = "Import successful.";
Comment on lines +448 to +450

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 StatusMessage guard misses two message variants

The condition !StatusMessage.Contains("Normalized") && !StatusMessage.Contains("GenLauncher") fails to protect two messages that were explicitly set earlier in the block:

  • Line 425: "Normalization warning: {error}. Import will continue." — starts with "Normalization" (not "Normalized") and contains no "GenLauncher", so the guard evaluates to true and overwrites it with "Import successful.".
  • Line 439 (catch block): "Import successful (normalization check failed)." — contains neither "Normalized" nor "GenLauncher", same result.

Both messages convey important information to the user (a warning or a failure) that will be silently hidden. A more robust fix is to use a dedicated boolean flag rather than inspecting the message string:

bool normalizationSetStatus = false;

// … inside the GenLauncher try/catch block, set normalizationSetStatus = true
// wherever a specific StatusMessage is assigned …

await RefreshStagingTreeAsync();

if (!normalizationSetStatus)
{
    StatusMessage = "Import successful.";
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: GenHub/GenHub/Features/GameProfiles/ViewModels/AddLocalContentViewModel.cs
Line: 445-447

Comment:
**StatusMessage guard misses two message variants**

The condition `!StatusMessage.Contains("Normalized") && !StatusMessage.Contains("GenLauncher")` fails to protect two messages that were explicitly set earlier in the block:

- Line 425: `"Normalization warning: {error}. Import will continue."` — starts with `"Normalization"` (not `"Normalized"`) and contains no `"GenLauncher"`, so the guard evaluates to `true` and overwrites it with `"Import successful."`.
- Line 439 (catch block): `"Import successful (normalization check failed)."` — contains neither `"Normalized"` nor `"GenLauncher"`, same result.

Both messages convey important information to the user (a warning or a failure) that will be silently hidden. A more robust fix is to use a dedicated boolean flag rather than inspecting the message string:

```csharp
bool normalizationSetStatus = false;

// … inside the GenLauncher try/catch block, set normalizationSetStatus = true
// wherever a specific StatusMessage is assigned …

await RefreshStagingTreeAsync();

if (!normalizationSetStatus)
{
    StatusMessage = "Import successful.";
}
```

How can I resolve this? If you propose a fix, please make it concise.

}

Validate();
}
catch (Exception ex)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public DemoAddLocalContentViewModel(
IContentStorageService? contentStorageService,
INotificationService? notificationService,
ILogger<AddLocalContentViewModel>? logger = null)
: base(localContentService ?? new MockLocalContentService(), contentStorageService, logger)
: base(localContentService ?? new MockLocalContentService(), contentStorageService, null, null, logger)
Comment thread
greptile-apps[bot] marked this conversation as resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Passing null for non-nullable services will cause NullReferenceException.

The AddLocalContentViewModel base constructor declares IGenLauncherNormalizationService and IDialogService as non-nullable parameters (see AddLocalContentViewModel.cs:26-31). More critically, ImportContentAsync (lines 378-403 in the base class) directly invokes genLauncherNormalizationService.DetectGenLauncherFilesAsync() and dialogService.ShowConfirmationAsync() without null guards.

If any code path in demo mode triggers content import, this will crash at runtime.

🛠️ Suggested approaches

Option 1: Create mock/no-op implementations for demo mode

-        : base(localContentService ?? new MockLocalContentService(), contentStorageService, null, null, logger)
+        : base(localContentService ?? new MockLocalContentService(), contentStorageService, new MockGenLauncherNormalizationService(), new MockDialogService(), logger)

Option 2: Add null guards in AddLocalContentViewModel.ImportContentAsync (preferred if demo mode should skip normalization entirely)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@GenHub/GenHub/Features/GameProfiles/ViewModels/DemoAddLocalContentViewModel.cs`
at line 32, The constructor for DemoAddLocalContentViewModel passes null for
IGenLauncherNormalizationService and IDialogService to the
AddLocalContentViewModel base which will cause NullReferenceException because
ImportContentAsync calls
genLauncherNormalizationService.DetectGenLauncherFilesAsync() and
dialogService.ShowConfirmationAsync(); fix by providing non-null no-op/mock
implementations instead of null (e.g., construct and pass a
MockGenLauncherNormalizationService and a MockDialogService or accept and
forward real instances), or alternatively update DemoAddLocalContentViewModel to
inject appropriate implementations and forward them to the base constructor so
AddLocalContentViewModel.ImportContentAsync always has valid
genLauncherNormalizationService and dialogService instances.

{
_notificationService = notificationService;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ public partial class DemoGameProfileSettingsViewModel : GameProfileSettingsViewM
/// <param name="manifestPool">The content manifest pool.</param>
/// <param name="contentStorageService">The content storage service.</param>
/// <param name="localContentService">The local content service.</param>
/// <param name="genLauncherNormalizationService">The GenLauncher normalization service.</param>
/// <param name="dialogService">The dialog service.</param>
/// <param name="logger">The logger for this view model.</param>
/// <param name="gameSettingsLogger">The logger for the game settings view model.</param>
public DemoGameProfileSettingsViewModel(
Expand All @@ -46,6 +48,8 @@ public DemoGameProfileSettingsViewModel(
IContentManifestPool? manifestPool,
IContentStorageService? contentStorageService,
ILocalContentService? localContentService,
IGenLauncherNormalizationService? genLauncherNormalizationService,
IDialogService? dialogService,
ILogger<GameProfileSettingsViewModel>? logger,
ILogger<GameSettingsViewModel>? gameSettingsLogger)
: base(
Expand All @@ -58,6 +62,8 @@ public DemoGameProfileSettingsViewModel(
manifestPool,
contentStorageService,
localContentService,
genLauncherNormalizationService,
dialogService,
logger,
gameSettingsLogger)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -668,7 +668,12 @@ private async Task AddLocalContentAsync(Avalonia.Controls.Window? owner)

if (dialogOwner == null) return;

var vm = new AddLocalContentViewModel(_localContentService, _contentStorageService, null);
var vm = new AddLocalContentViewModel(
_localContentService,
_contentStorageService,
_genLauncherNormalizationService,
_dialogService,
null);
Comment on lines +671 to +676

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Consider null guard before creating AddLocalContentViewModel.

The fields _genLauncherNormalizationService and _dialogService are nullable, but AddLocalContentViewModel's constructor expects non-nullable parameters. If DI fails to resolve these services, this will throw NullReferenceException.

🛡️ Optional defensive check
+            if (_genLauncherNormalizationService == null || _dialogService == null)
+            {
+                _logger?.LogWarning("GenLauncher normalization or dialog service unavailable");
+                // Fall through to create VM without normalization support, or return early
+            }
+
             var vm = new AddLocalContentViewModel(
                 _localContentService,
                 _contentStorageService,
-                _genLauncherNormalizationService,
-                _dialogService,
+                _genLauncherNormalizationService!,
+                _dialogService!,
                 null);

Alternatively, ensure AddLocalContentViewModel handles null services gracefully (with null checks in ImportContentAsync).

📝 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.

Suggested change
var vm = new AddLocalContentViewModel(
_localContentService,
_contentStorageService,
_genLauncherNormalizationService,
_dialogService,
null);
if (_genLauncherNormalizationService == null || _dialogService == null)
{
_logger?.LogWarning("GenLauncher normalization or dialog service unavailable");
// Fall through to create VM without normalization support, or return early
}
var vm = new AddLocalContentViewModel(
_localContentService,
_contentStorageService,
_genLauncherNormalizationService!,
_dialogService!,
null);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@GenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileSettingsViewModel.Commands.cs`
around lines 671 - 676, The code creates a new AddLocalContentViewModel with
possibly null dependencies (_genLauncherNormalizationService and _dialogService)
which will throw if DI failed; add a null guard before constructing the VM:
check that _genLauncherNormalizationService and _dialogService are not null (and
_localContentService/_contentStorageService if relevant) and handle the null
case (early return, show error via existing dialog/logging service, or throw a
clear ArgumentNullException), or alternatively update AddLocalContentViewModel
to accept nullable services and perform defensive checks in its
ImportContentAsync method; locate the creation site that calls new
AddLocalContentViewModel(...) to implement the guard or update the
AddLocalContentViewModel constructor/ImportContentAsync to handle nulls.

var window = new Views.AddLocalContentWindow
{
DataContext = vm,
Expand Down Expand Up @@ -724,7 +729,12 @@ private async Task EditContent(ContentDisplayItem? contentItem)

if (owner == null) return;

var vm = new AddLocalContentViewModel(_localContentService, _contentStorageService, null);
var vm = new AddLocalContentViewModel(
_localContentService,
_contentStorageService,
_genLauncherNormalizationService,
_dialogService,
null);
Comment on lines +732 to +737

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Same null-safety concern applies to EditContent.

This mirrors the issue in AddLocalContentAsync — nullable fields passed to non-nullable constructor parameters.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@GenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileSettingsViewModel.Commands.cs`
around lines 732 - 737, EditContent is creating an AddLocalContentViewModel with
a literal null for a parameter that is non-nullable (same as in
AddLocalContentAsync); update the call inside EditContent to pass a valid value
instead of null (e.g., the existing content/model or the selected item the edit
is for) or refactor AddLocalContentViewModel to accept a nullable for that
parameter and handle null internally; locate the construction of
AddLocalContentViewModel in EditContent and either supply the appropriate
existingContent variable or change the constructor/overload to accept and safely
handle nulls.

await vm.LoadFromManifestAsync(contentItem);

var window = new Views.AddLocalContentWindow
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,8 @@ private static ContentDisplayItem ConvertToViewModelContentDisplayItem(Core.Mode
private readonly IContentManifestPool? _manifestPool;
private readonly IContentStorageService? _contentStorageService;
private readonly ILocalContentService? _localContentService;
private readonly IGenLauncherNormalizationService? _genLauncherNormalizationService;
private readonly IDialogService? _dialogService;
private readonly ILogger<GameProfileSettingsViewModel>? _logger;
private readonly ILogger<GameSettingsViewModel>? _gameSettingsLogger;

Expand Down Expand Up @@ -182,6 +184,8 @@ private static ContentDisplayItem ConvertToViewModelContentDisplayItem(Core.Mode
/// <param name="manifestPool">The manifest pool.</param>
/// <param name="contentStorageService">The content storage service.</param>
/// <param name="localContentService">The local content service.</param>
/// <param name="genLauncherNormalizationService">The GenLauncher normalization service.</param>
/// <param name="dialogService">The dialog service.</param>
/// <param name="logger">The logger for this view model.</param>
/// <param name="gameSettingsLogger">The logger for the game settings view model.</param>
public GameProfileSettingsViewModel(
Expand All @@ -194,6 +198,8 @@ public GameProfileSettingsViewModel(
IContentManifestPool? manifestPool,
IContentStorageService? contentStorageService,
ILocalContentService? localContentService,
IGenLauncherNormalizationService? genLauncherNormalizationService,
IDialogService? dialogService,
ILogger<GameProfileSettingsViewModel>? logger,
ILogger<GameSettingsViewModel>? gameSettingsLogger)
Comment on lines +201 to 204

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify test files have been updated with new constructor parameters
rg -n 'new GameProfileSettingsViewModel\(' --type cs -g '*Test*.cs' -A 12 | head -80

Repository: community-outpost/GenHub

Length of output: 11463


🏁 Script executed:

cat GenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileSettingsViewModel.cs | grep -n "public GameProfileSettingsViewModel" -A 20

Repository: community-outpost/GenHub

Length of output: 1230


🏁 Script executed:

sed -n '191,204p' GenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileSettingsViewModel.cs

Repository: community-outpost/GenHub

Length of output: 818


Test files require updates for constructor parameter changes.

The constructor now requires 13 parameters including the two new IGenLauncherNormalizationService and IDialogService parameters, but all test instantiations (in GameProfileSettingsViewModelTests.cs, GameProfileSettingsViewModelDependencyTests.cs, GameProfileLauncherViewModelTests.cs, and MainViewModelTests.cs) pass only 11 parameters. These will fail to compile until updated.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@GenHub/GenHub/Features/GameProfiles/ViewModels/GameProfileSettingsViewModel.cs`
around lines 201 - 204, Update all test instantiations to match the new
GameProfileSettingsViewModel constructor signature by providing the two new
dependencies: an IGenLauncherNormalizationService and an IDialogService instance
(or suitable test doubles/mocks). Locate places constructing
GameProfileSettingsViewModel in GameProfileSettingsViewModelTests.cs,
GameProfileSettingsViewModelDependencyTests.cs,
GameProfileLauncherViewModelTests.cs, and MainViewModelTests.cs and add
mock/fake implementations for IGenLauncherNormalizationService and
IDialogService to the constructor calls so the total parameters passed is 13 and
the signatures match the class constructor.

{
Expand All @@ -206,6 +212,8 @@ public GameProfileSettingsViewModel(
_manifestPool = manifestPool;
_contentStorageService = contentStorageService;
_localContentService = localContentService;
_genLauncherNormalizationService = genLauncherNormalizationService;
_dialogService = dialogService;
_logger = logger;
_gameSettingsLogger = gameSettingsLogger;

Expand Down
Loading
Loading