Skip to content

Search nested mods to match in-game mod manager behavior #3527

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Lightningbulb2 wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Search nested mods to match in-game mod manager behavior #3527

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 11 additions & 5 deletions src/main/java/com/faforever/client/mod/ModService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,27 +185,33 @@ private void loadInstalledMods() {
@Override
protected Void call() {
updateTitle(i18n.get("modVault.loadingMods"));
try (Stream<Path> customModsDirectory = list(forgedAlliancePrefs.getModsDirectory())) {
List<Path> modPaths = new ArrayList<>();
customModsDirectory.collect(toCollection(() -> modPaths));
Path modsDirectory = forgedAlliancePrefs.getModsDirectory();

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));
Comment on lines +190 to +195

@coderabbitai coderabbitai Bot Jun 8, 2026 •
edited
Loading

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 | ⚡ Quick win

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"))).

@Lightningbulb2 Lightningbulb2 Jun 8, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

@coderabbitai coderabbitai Bot Jun 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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


long totalMods = modPaths.size();
long modsRead = 0;

for (Path modPath : modPaths) {
updateProgress(++modsRead, totalMods);
try {
addInstalledMod(modPath);
} catch (Exception e) {
log.warn("Corrupt mod: `{}`", modPath, e);

notificationService.addPersistentWarnNotification(
List.of(new Action(i18n.get("corruptedMods.show"), () -> platformService.reveal(modPath))),
"corruptedModsError.notification", modPath.getFileName());
}
}
} catch (IOException e) {
log.error("Mods could not be read from: `{}`", forgedAlliancePrefs.getModsDirectory(), e);
log.error("Mods could not be read from: `{}`", modsDirectory, e);
}

return null;
}
});
Expand Down