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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis change extracts command construction in Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/cryptomator/macos/revealpath/OpenCmdRevealPathService.java (1)
24-34:⚠️ Potential issue | 🟠 MajorTreat
waitFortimeout as a failure.If the
openprocess does not exit within 5 seconds, this method currently returns successfully and leaves the subprocess running. That turns a hung reveal into a silent success.Suggested fix
final var process = new ProcessBuilder().command(cmd).start(); try (var reader = process.errorReader()) { - if (process.waitFor(5000, TimeUnit.MILLISECONDS)) { - int exitValue = process.exitValue(); - if (process.exitValue() != 0) { - String error = reader.lines().collect(Collectors.joining()); - throw new RevealFailedException("open command exited with value " + exitValue + " and error message: " + error); - } - } + if (!process.waitFor(5000, TimeUnit.MILLISECONDS)) { + process.destroyForcibly(); + throw new RevealFailedException("open command timed out after 5000 ms"); + } + int exitValue = process.exitValue(); + if (exitValue != 0) { + String error = reader.lines().collect(Collectors.joining()); + throw new RevealFailedException("open command exited with value " + exitValue + " and error message: " + error); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/cryptomator/macos/revealpath/OpenCmdRevealPathService.java` around lines 24 - 34, The method in OpenCmdRevealPathService uses process.waitFor(5000, TimeUnit.MILLISECONDS) but ignores the case when it returns false, causing a hung subprocess to be treated as success; update the logic in the block around createCommandAsList(p) / new ProcessBuilder().command(cmd).start() so that if waitFor(...) returns false you treat it as a failure: destroy/terminate the process, read any available error/output via process.errorReader() (and/or inputReader), and throw a RevealFailedException that indicates the timeout; only call process.exitValue() when waitFor returned true and use that exit value and collected error text in the exception message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@src/main/java/org/cryptomator/macos/revealpath/OpenCmdRevealPathService.java`:
- Around line 24-34: The method in OpenCmdRevealPathService uses
process.waitFor(5000, TimeUnit.MILLISECONDS) but ignores the case when it
returns false, causing a hung subprocess to be treated as success; update the
logic in the block around createCommandAsList(p) / new
ProcessBuilder().command(cmd).start() so that if waitFor(...) returns false you
treat it as a failure: destroy/terminate the process, read any available
error/output via process.errorReader() (and/or inputReader), and throw a
RevealFailedException that indicates the timeout; only call process.exitValue()
when waitFor returned true and use that exit value and collected error text in
the exception message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c460ca78-94fa-43f4-b065-a7d00d17577f
📒 Files selected for processing (1)
src/main/java/org/cryptomator/macos/revealpath/OpenCmdRevealPathService.java
1 participant
This PR fixes the macOS specific
OpenCmdRevealPathService.That RevealPathService implementation uses the macOS based
openbinary. When callingrevealinternally the impl constructs aopen ... /some/pathcommand and calls it as a subprocess.The impl contained a bug: If the given path is a directory, the constructed command contained three parameters:
open""(empty string)It is fixed by constructing the correct command. (not containing the emtpy string)