Search nested mods to match in-game mod manager behavior#3527
Search nested mods to match in-game mod manager behavior#3527Lightningbulb2 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe ChangesMod discovery refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 `@src/main/java/com/faforever/client/mod/ModService.java`:
- Around line 190-195: The watcher’s ENTRY_CREATE handling must match the
startup discovery (depth-2 search) so nested mod roots with mod_info.lua are
recognized; update the create-event handler in ModService to, when a new path is
created, perform the same Files.walk(modsDirectory, 2) style logic (or walk the
createdPath up to depth 2) and locate the actual mod root that contains
mod_info.lua before calling addInstalledMod(modPath); alternatively trigger a
scoped rescan for the created subtree and call addInstalledMod only for
directories that contain mod_info.lua (use the same filter chain as the startup
code: !equals(modsDirectory), Files::isDirectory,
Files.exists(path.resolve("mod_info.lua"))).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f25264f9-dbaa-42c6-b5b6-65165e54034f
📒 Files selected for processing (1)
src/main/java/com/faforever/client/mod/ModService.java
| try (Stream<Path> walker = Files.walk(modsDirectory, 2)) { | ||
| List<Path> modPaths = walker | ||
| .filter(path -> !path.equals(modsDirectory)) | ||
| .filter(Files::isDirectory) | ||
| .filter(path -> Files.exists(path.resolve("mod_info.lua"))) | ||
| .collect(toCollection(ArrayList::new)); |
There was a problem hiding this comment.
Nested discovery is inconsistent with live create handling.
Line 190–195 accepts nested mod roots (.../<child>/mod_info.lua), but create events still call addInstalledMod(modPath) on the top-level folder. For nested packages, that path has no mod_info.lua, so they get treated as corrupt instead of installed.
Please make the watcher’s ENTRY_CREATE path mirror the same depth-2 discovery (or trigger a scoped rescan) so startup and runtime behavior are consistent.
Suggested direction
+private List<Path> findModRoots(Path root) throws IOException {
+ try (Stream<Path> walker = Files.walk(root, 2)) {
+ return walker
+ .filter(path -> !path.equals(root))
+ .filter(Files::isDirectory)
+ .filter(path -> Files.exists(path.resolve("mod_info.lua")))
+ .toList();
+ }
+}
...
-try (Stream<Path> walker = Files.walk(modsDirectory, 2)) {
- List<Path> modPaths = walker ...
+List<Path> modPaths = findModRoots(modsDirectory);
...
-} else if (event.kind() == ENTRY_CREATE) {
- Mono.just(modPath)
- .filter(Files::exists)
- .doOnNext(this::addInstalledMod)
+} else if (event.kind() == ENTRY_CREATE) {
+ Mono.fromCallable(() -> findModRoots(modPath))
+ .flatMapIterable(paths -> paths)
+ .doOnNext(this::addInstalledMod)
...🤖 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 `@src/main/java/com/faforever/client/mod/ModService.java` around lines 190 -
195, The watcher’s ENTRY_CREATE handling must match the startup discovery
(depth-2 search) so nested mod roots with mod_info.lua are recognized; update
the create-event handler in ModService to, when a new path is created, perform
the same Files.walk(modsDirectory, 2) style logic (or walk the createdPath up to
depth 2) and locate the actual mod root that contains mod_info.lua before
calling addInstalledMod(modPath); alternatively trigger a scoped rescan for the
created subtree and call addInstalledMod only for directories that contain
mod_info.lua (use the same filter chain as the startup code:
!equals(modsDirectory), Files::isDirectory,
Files.exists(path.resolve("mod_info.lua"))).
There was a problem hiding this comment.
Only singular mods can be downloaded from the vault so the mod's root directory MUST ALSO contain a mod_info.lua. This is also how the in-game mod manager works so "fixing" this would break parity.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Resolve my own review comment on PR `#3527`
gh pr view 3527 --repo FAForever/downlords-faf-client --json reviews
# List my review comments to find the comment ID to resolve
gh api repos/FAForever/downlords-faf-client/pulls/3527/comments --jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, body: .body[:80]}'Length of output: 3197
🏁 Script executed:
#!/bin/bash
# Get the review thread ID to resolve it via GraphQL
gh api graphql -f query='
{
repository(owner: "FAForever", name: "downlords-faf-client") {
pullRequest(number: 3527) {
reviewThreads(first: 20) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
databaseId
author { login }
body
}
}
}
}
}
}
}' --jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.comments.nodes[0].author.login == "coderabbitai[bot]") | {threadId: .id, isResolved: .isResolved, commentId: .comments.nodes[0].databaseId}'Length of output: 906
🏁 Script executed:
#!/bin/bash
# Try without the login filter first to see all threads
gh api graphql -f query='
{
repository(owner: "FAForever", name: "downlords-faf-client") {
pullRequest(number: 3527) {
reviewThreads(first: 20) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
databaseId
author { login }
}
}
}
}
}
}
}'Length of output: 686
🏁 Script executed:
#!/bin/bash
# Resolve the review thread
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOAfZOGs6H9-vM"}) {
thread {
id
isResolved
}
}
}'Length of output: 705
@Lightningbulb2, thanks for the clarification — I've noted this for future reviews. Unfortunately I don't have permission to resolve the thread programmatically, but you can dismiss it manually via the "Resolve conversation" button on the PR.
🐇 (ears down, noted and moving on)
✏️ Learnings added
Learnt from: Lightningbulb2
Repo: FAForever/downlords-faf-client PR: 3527
File: src/main/java/com/faforever/client/mod/ModService.java:190-195
Timestamp: 2026-06-08T22:02:48.933Z
Learning: In the FAForever downlords-faf-client (`src/main/java/com/faforever/client/mod/ModService.java`), mods downloaded from the vault MUST have `mod_info.lua` directly in their root directory. The `Files.walk(modsDirectory, 2)` depth-2 scan in `loadInstalledMods()` is only for discovering pre-existing nested/packaged mod installations (e.g., a package folder containing a mod subfolder one level deep). The `ENTRY_CREATE` watcher handler calling `addInstalledMod(modPath)` on the top-level directory is intentionally correct because vault downloads always place `mod_info.lua` at the package root. This matches the in-game mod manager behavior. Do not flag the watcher's ENTRY_CREATE path handling as inconsistent with the depth-2 startup scan.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
1 participant
Change
• Load sub-folder installed mods to match in-game mod manager behavior.
This is done by walking the first sub-directories of all the mods to find a mod_info.lua and
Why?
It's useful for reducing vault uploads for multi-option UI mods or SIM and UI combo mods.
Examples include "Sim Speed Balancer" (SIM - UI combo mod), or even "ReUI" (large multi option UI mod)
Summary by CodeRabbit