Allow Choosing Branch for Mod in Mod Manager#1262
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds branch selection from the install dialog through GitHub mod installation, updates mod path lookup across game and collection locations, and changes update handling for collection moves. It also adds a branch-specific validation exception and refactors collection helper methods. ChangesBranch-aware mod installation and collection path fixes
Sequence Diagram(s)sequenceDiagram
participant MainViewModel
participant ModsService
participant GitHub
participant LibGit2Sharp
MainViewModel->>ModsService: InstallMod(name, branchName)
ModsService->>ModsService: derive branchName from owner/repo/branch
ModsService->>GitHub: validate mod.yml on selected branch
alt overwrite existing mod
ModsService->>ModsService: CleanModFiles()
ModsService->>ModsService: ClearOldMods()
end
ModsService->>LibGit2Sharp: Clone with BranchName
LibGit2Sharp-->>ModsService: transfer progress
ModsService-->>MainViewModel: install complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@OpenKh.Tools.ModsManager/Services/ModsService.cs`:
- Around line 286-298: The issue is that when repositoryName has 3 parts
(owner/repo/branch) but branchName is already explicitly provided, the condition
checking string.IsNullOrWhiteSpace(branchName) prevents normalization of
repositoryName, leaving it with the invalid 3-part format. This causes
RepositoryService.IsFileExists to receive an incorrect repository path. Modify
the logic to always extract and normalize repositoryName to only the owner/repo
portion whenever it has 3 parts, then use the explicit branchName if provided,
or fall back to the extracted branch part if branchName is empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0912879f-19b7-42bd-a985-1e3a5fc4865e
📒 Files selected for processing (3)
OpenKh.Tools.ModsManager/Services/ModsService.csOpenKh.Tools.ModsManager/ViewModels/MainViewModel.csOpenKh.Tools.ModsManager/Views/InstallModView.xaml.cs
|
Currently this PR breaks installing over a previously installed mod with this error. 
not sure why this is happening but its consistent if i install a mod from github then immediately install it again after clicking yes to overwrite it i get this error even when stepping through in debug. the fix for this previously was the await Task.Delay(1) but its seems broken by this somehow. also I believe the intention is to allow github inputs like I did look into this previously but did not finish anything because my first step was to make a second input box on the UI for the user to type the branch into and if blank it would do main branch. personally I'd prefer if this PR did something similar with editing the UI since this is technically not how the branch URL looks and a separate input box would be preferred. I wont deny it based on this but you will need to look into recreating the error I am seeing and fixing it if im the only one seeing it maybe its a me problem but if its happening to me i imagine itll happen to people when this is released causing headaches. |
Couldn't he just do Directory.Exists(targetDir) => Directory.Delete(targetDir)? |
Fix the mod overwrite bug Add the branch textbox
|
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)
OpenKh.Tools.ModsManager/Services/ModsService.cs (1)
512-515: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWait for collection moves and keep the destination guard in
Update.
MoveFromCollection/MoveToCollectionreturnTask, butUpdatedrops them. Also, unlikeFetchUpdates, this path callsMoveToCollectioneven when the collection destination already exists.Proposed fix
var metadata = GetMetadata(modPath); if (isCollection && !metadata.IsCollection) - MoveFromCollection(modName); + MoveFromCollection(modName).GetAwaiter().GetResult(); else if (!isCollection && metadata.IsCollection) - MoveToCollection(modName); + { + if (!Directory.Exists(GetCollectionPath(modName))) + MoveToCollection(modName).GetAwaiter().GetResult(); + else + CleanModFiles(GetModPath(modName)).GetAwaiter().GetResult(); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@OpenKh.Tools.ModsManager/Services/ModsService.cs` around lines 512 - 515, The Update method is not awaiting the calls to MoveFromCollection and MoveToCollection even though these methods return Task, which causes the async operations to be dropped. Additionally, the MoveToCollection call is missing a guard condition that checks whether the destination collection already exists before attempting the move, unlike what is implemented in FetchUpdates. Fix this by adding await keywords before both MoveFromCollection(modName) and MoveToCollection(modName) calls to ensure the async operations complete, and add a destination guard condition (similar to the guard in FetchUpdates) before calling MoveToCollection to prevent unnecessary moves when the collection already exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@OpenKh.Tools.ModsManager/Services/ModsService.cs`:
- Line 335: The ClearOldMods method is returning before all its cleanup tasks
complete because nested CleanModFiles calls or their returned tasks are not
being properly awaited. Examine the ClearOldMods method implementation and
ensure all Task-returning calls within it, particularly any CleanModFiles
invocations, are properly awaited so the method waits for all cleanup operations
to finish before returning. Apply the same fix to the code regions around lines
636-663 where similar fire-and-forget patterns exist.
- Around line 286-292: The current condition checking parts.Length == 3 only
handles simple branch names without slashes, causing shorthand names like
owner/repo/feature/foo to skip normalization. Change the condition to
parts.Length >= 3 or parts.Length > 2 to handle branch names that contain
slashes. When this condition is true, reconstruct the branch name by joining all
segments after the first two parts back together with forward slashes to
preserve any slashes in the branch name itself, then set repositoryName to just
the first two parts joined together.
---
Outside diff comments:
In `@OpenKh.Tools.ModsManager/Services/ModsService.cs`:
- Around line 512-515: The Update method is not awaiting the calls to
MoveFromCollection and MoveToCollection even though these methods return Task,
which causes the async operations to be dropped. Additionally, the
MoveToCollection call is missing a guard condition that checks whether the
destination collection already exists before attempting the move, unlike what is
implemented in FetchUpdates. Fix this by adding await keywords before both
MoveFromCollection(modName) and MoveToCollection(modName) calls to ensure the
async operations complete, and add a destination guard condition (similar to the
guard in FetchUpdates) before calling MoveToCollection to prevent unnecessary
moves when the collection already exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3557ec32-7bbd-4b2a-8428-7216676114b6
📒 Files selected for processing (2)
OpenKh.Tools.ModsManager/Services/ModsService.csOpenKh.Tools.ModsManager/Views/InstallModView.xaml
✅ Files skipped from review due to trivial changes (1)
- OpenKh.Tools.ModsManager/Views/InstallModView.xaml
I did not make the Delay(1) addition that was needed for the mod collections PR. When I gave users the ability to overwrite the mod in my PR with the dialogue box etc. I never ran into any error of the folder existing once I had fixed it. I do not remember what i did id have to look at my PR and the code paths very well could've changed since then especially with the collections PR. I also saw no reports from when I added the option to overwrite (again once i fixed all the errors which happened before the PR was made). basically before this PR any folder conflict like this didn't show in my testing and I saw no reports. Also since its a github clone we need to change some folder/file permissions before deleting them otherwise an error happens which is what the called method does change permissions then deletes the files. |
I didn't say you made it or no, I was just asking if this fix was a possibility. GICU has insured it was. I did not say anything to insult or offend, I swear. |
|
I take no offense just wanted to clarify the history of replacing a mod that's already installed since I made it and when it was implemented it was working since its been through a couple changes. |
I have never encountered this, aren't we using Octokit? |
There was a problem hiding this comment.
Just my one requested change and everything works for me and is good
In my head ideally branch name would be a semi small box on the right side of where you put in the repo but ideally I think it would be hard to make look good so that's fine as is.
| progressOutput?.Invoke($"Fetching file {ModMetadata} from branch {branchName}"); | ||
| var isValidMod = await RepositoryService.IsFileExists(repositoryName, branchName, ModMetadata); | ||
| if (!isValidMod) | ||
| throw new ModNotValidException(repositoryName); |
There was a problem hiding this comment.
To separate a possible bad branch from a bad repo entirely this should be a new error i did a quick mock up mostly copying the error you are currently using. changed this to throw new BranchNotValidException(repositoryName, branchName);
Then in Exceptions.cs (since i cant review a file you didn't modify I added this as a new exception. You can adjust the error message if you want I think its fine as is but I don't care either way
public class BranchNotValidException : Exception
{
public string ModName { get; }
public string BranchName { get; }
public BranchNotValidException(string modName, string branchName) :
base($"The branch '{branchName}' is not valid for '{modName}'.\nOr The mod '{modName}' does not contain a valid OpenKH compatible mod due to the missing 'mod.yml' file.")
{
ModName = modName;
BranchName = branchName;
}
}
3 participants
This should allow choosing a branch apart from the default in the Mod Manager. Useful for allowing others to test things without requiring the mod is updated or pointing the user to another repository.