Add simple permission checks before plugin uninstall #390
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds a utility to capture throwable stack traces as strings, uses it in TaskRunner error reporting, disables delete/uninstall UI buttons when target paths are not writable, and avoids inserting null root items into the plugin tree view during refresh. Changes
Sequence Diagram(s)sequenceDiagram
participant TaskRunner
participant TaskBarController
participant StringUtils
participant Throwable as Exception
TaskRunner->>Exception: catches exception
TaskRunner->>StringUtils: getStackTraceAsString(Exception)
StringUtils-->>TaskRunner: stack trace string
TaskRunner->>TaskBarController: setErrorLog(message, Exception.getMessage(), stackTraceString)
Note right of TaskBarController #DDDDFF: UI receives full stack trace for display
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
owlplug-client/src/main/java/com/owlplug/plugin/controllers/DirectoryInfoController.java (1)
51-51: Remove unused import.The
Tooltipimport is not used anywhere in this file.-import javafx.scene.control.Tooltip;owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginInfoController.java (1)
175-176: Permission check aligns with PR objective but could benefit from user feedback.The implementation is consistent with
DirectoryInfoControllerand provides a useful UI hint. However, it shares the same platform-specific limitations wherecanWrite()may not accurately reflect actual deletion permissions.Consider adding a tooltip to explain why the uninstall button is disabled when permissions are insufficient.
File file = new File(plugin.getPath()); this.uninstallButton.setDisable(!file.canWrite()); +if (!file.canWrite()) { + this.uninstallButton.setTooltip(new Tooltip("Insufficient permissions to uninstall this plugin")); +}owlplug-client/src/main/java/com/owlplug/core/utils/StringUtils.java (1)
46-51: Handle null input and consider including exception cause chain.The method will throw
NullPointerExceptionif a null throwable is passed. Additionally, it doesn't include the cause chain fromgetCause(), which may omit valuable debugging information.Consider using the standard approach with
StringWriterandPrintWriter, or add null checks and cause chain handling.public static String getStackTraceAsString(Throwable throwable) { + if (throwable == null) { + return ""; + } - return throwable.toString() + "\n" - + Arrays.stream(throwable.getStackTrace()) - .map(StackTraceElement::toString) - .collect(Collectors.joining("\n")); + java.io.StringWriter sw = new java.io.StringWriter(); + throwable.printStackTrace(new java.io.PrintWriter(sw)); + return sw.toString(); }This approach automatically includes the cause chain and handles all edge cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
owlplug-client/src/main/java/com/owlplug/core/components/TaskRunner.java(2 hunks)owlplug-client/src/main/java/com/owlplug/core/utils/StringUtils.java(2 hunks)owlplug-client/src/main/java/com/owlplug/plugin/controllers/DirectoryInfoController.java(3 hunks)owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginInfoController.java(1 hunks)owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginTreeViewController.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginTreeViewController.java (2)
158-165: Good defensive programming to prevent empty tree items.The null check ensures that only directory roots containing plugins are added to the tree view, preventing empty nodes from appearing in the UI.
258-258: Grammar correction improves documentation clarity.owlplug-client/src/main/java/com/owlplug/core/components/TaskRunner.java (1)
127-127: Enhanced error logging improves debugging capability.The change from
ex.toString()toStringUtils.getStackTraceAsString(ex)provides complete stack traces in error logs, making it significantly easier to diagnose issues in production.owlplug-client/src/main/java/com/owlplug/plugin/controllers/DirectoryInfoController.java (1)
171-172: Permission check is a necessary safeguard, not merely a UX hint.The
canWrite()check preventsDirectoryRemoveTaskfrom executing when the directory is not writable. UnlikePluginRemoveTaskwhich handlesIOExceptiongracefully,DirectoryRemoveTasklacks exception handling and will propagate deletion errors uncaught. The permission check is essential to prevent unhandled exceptions.However, note that
canWrite()has platform-specific limitations: on Windows it checks read-only attributes rather than full NTFS permissions, and it doesn't account for parent directory write requirements for deletion. Despite these limitations, the check appropriately prevents most permission-denied scenarios at the UI layer.Likely an incorrect or invalid review comment.
DropSnorz
force-pushed
the
feat/delete-permissions
branch
from
6c4a993 to
dc820c2
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
owlplug-client/src/main/java/com/owlplug/core/components/TaskRunner.java(2 hunks)owlplug-client/src/main/java/com/owlplug/core/utils/StringUtils.java(2 hunks)owlplug-client/src/main/java/com/owlplug/plugin/controllers/DirectoryInfoController.java(2 hunks)owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginInfoController.java(1 hunks)owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginTreeViewController.java(2 hunks)owlplug-client/src/test/java/com/owlplug/core/utils/StringUtilsTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- owlplug-client/src/main/java/com/owlplug/core/components/TaskRunner.java
- owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginInfoController.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-09T18:03:46.895Z
Learnt from: DropSnorz
PR: DropSnorz/OwlPlug#280
File: owlplug-client/src/main/java/com/owlplug/core/controllers/dialogs/DisablePluginDialogController.java:47-49
Timestamp: 2025-01-09T18:03:46.895Z
Learning: In the DisablePluginDialogController, the admin privileges warning is always shown when disabling a plugin, regardless of the actual file permissions, as implementing permission checks is complex and OS-dependent.
Applied to files:
owlplug-client/src/main/java/com/owlplug/plugin/controllers/DirectoryInfoController.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (7)
owlplug-client/src/main/java/com/owlplug/plugin/controllers/DirectoryInfoController.java (2)
35-35: LGTM!The import is necessary for the permission check implementation.
170-171: Good UI safeguard, though platform-dependent.The
canWrite()check provides useful immediate feedback by disabling the delete button when the directory appears non-writable. Note thatFile.canWrite()behavior is platform-dependent and may not catch all permission scenarios, especially on Windows. Users may still encounter permission errors during the actual deletion if the check yields a false positive. This approach aligns with the project's strategy of using simple permission checks rather than complex OS-dependent verification. Based on learnings.owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginTreeViewController.java (2)
159-164: LGTM!The null check appropriately prevents empty directory roots from appearing in the tree view when no plugins are detected in that directory. This improves the user experience by keeping the tree clean.
258-258: LGTM!Grammar correction improves the documentation quality.
owlplug-client/src/main/java/com/owlplug/core/utils/StringUtils.java (1)
21-22: LGTM!The imports are necessary for the stack trace utility implementation.
owlplug-client/src/test/java/com/owlplug/core/utils/StringUtilsTest.java (2)
14-43: LGTM!The truncate tests provide comprehensive coverage including normal cases and edge cases (null input, negative size, suffix length handling).
47-70: LGTM!The ellipsis tests thoroughly cover the method's behavior including edge cases.
owlplug-client/src/test/java/com/owlplug/core/utils/StringUtilsTest.java
Show resolved
Hide resolved
DropSnorz
force-pushed
the
feat/delete-permissions
branch
from
dc820c2 to
fd7df0e
Compare
…ning all the updates from the original repository until the commit below: *********************************************************************** Merge pull request DropSnorz#390 from DropSnorz/feat/delete-permissions Add simple permission checks before plugin uninstall 422044a Arthur Poiret <[email protected]> on 30/10/2025 at 20:17 committed by GitHub <[email protected]> ***********************************************************************
Relates to #281