Plugin bundle installation sequence #371
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 WalkthroughThe PR removes central dialog stack management, moves dialog lifecycle to instance-scoped controllers, adds a new InstallStepDialogController and FXML for a multi-step installation flow, updates the view registry to preload the install view, and simplifies ExploreController to delegate installs to the new dialog controller. Changes
Sequence DiagramsequenceDiagram
actor User
participant ExploreController
participant InstallStepDialogController
participant DirectoryChooser
participant ExploreTaskFactory
participant AbstractDialogController
User->>ExploreController: request install(bundle)
ExploreController->>InstallStepDialogController: install(bundle)
InstallStepDialogController->>InstallStepDialogController: prepareInstallation()
InstallStepDialogController->>InstallStepDialogController: selectInstallationDirectory()
alt user opens chooser
InstallStepDialogController->>DirectoryChooser: show()
DirectoryChooser->>InstallStepDialogController: selected path
end
InstallStepDialogController->>InstallStepDialogController: verifyInstallationDirectory()
InstallStepDialogController->>ExploreTaskFactory: schedule BundleInstallTask
rect rgb(220,235,255)
Note right of AbstractDialogController: Dialog lifecycle handled per controller instance
InstallStepDialogController->>AbstractDialogController: close()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
DropSnorz
force-pushed
the
fix/plugin-install-popup
branch
4 times, most recently
from
f40d900 to
67d1ea4
Compare
DropSnorz
force-pushed
the
fix/plugin-install-popup
branch
from
67d1ea4 to
da5d017
Compare
|
@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: 8
🧹 Nitpick comments (3)
owlplug-client/src/main/java/com/owlplug/explore/controllers/dialogs/InstallStepDialogController.java (2)
131-142: Improve DirectoryChooser UX by setting an initial directory.Preselect the current or base directory to reduce navigation.
- DirectoryChooser directoryChooser = new DirectoryChooser(); + DirectoryChooser directoryChooser = new DirectoryChooser(); + File current = installParams.getInstallationDirectory(); + if (current != null && (current.isDirectory() || (current.getParentFile() != null && current.getParentFile().isDirectory()))) { + directoryChooser.setInitialDirectory(current.isDirectory() ? current : current.getParentFile()); + }
162-167: Disable format toggles not present in the bundle to prevent selecting irrelevant formats.The suggested enhancement is valid. All referenced toggle button fields (
vst2ToggleButton,vst3ToggleButton,auToggleButton,lv2ToggleButton) exist in the controller, andbundle.getFormats()correctly returnsList<String>containing the normalized format identifiers ("vst2", "vst3", "au", "lv2"). The null check forbundleis appropriate. Implement the suggested refactor to improve user experience by disabling format options unavailable in the package bundle.owlplug-client/src/main/resources/fxml/dialogs/InstallStepView.fxml (1)
26-26: Progress bar sizing and button semantics are appropriate UX improvements.Icon paths are correct and resolve to existing assets. The progress bar width constraint (550.0 > dialog width 500.0) should be replaced with responsive sizing using maxWidth, and the buttons should specify default/cancel semantics for standard keyboard navigation.
📜 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/DialogManager.java(0 hunks)owlplug-client/src/main/java/com/owlplug/core/components/LazyViewRegistry.java(2 hunks)owlplug-client/src/main/java/com/owlplug/core/controllers/dialogs/AbstractDialogController.java(3 hunks)owlplug-client/src/main/java/com/owlplug/explore/controllers/ExploreController.java(3 hunks)owlplug-client/src/main/java/com/owlplug/explore/controllers/dialogs/InstallStepDialogController.java(1 hunks)owlplug-client/src/main/resources/fxml/dialogs/InstallStepView.fxml(1 hunks)
💤 Files with no reviewable changes (1)
- owlplug-client/src/main/java/com/owlplug/core/components/DialogManager.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 (2)
owlplug-client/src/main/java/com/owlplug/core/components/LazyViewRegistry.java (1)
46-46: INSTALL_STEP_VIEW registration verified—all resources are available.The FXML file and icon assets referenced in the constant registration and preload call (lines 46 and 72) all exist and will resolve correctly at runtime. No issues found.
owlplug-client/src/main/java/com/owlplug/explore/controllers/ExploreController.java (1)
90-91: Singleton controller maintains mutable state; concurrency risk is valid but context-dependent.The review comment's concern is factually grounded.
InstallStepDialogControlleris a@Controllersingleton that maintains mutable instance state (installParams, UI components,toggleGroup,dialog). Theinstall()method is non-blocking and reuses the same dialog instance. If concurrentinstall()calls occur, state mutations will race and conflict.The
reset()method mitigates but doesn't prevent re-entry issues—it clears UI state butinstallParamsis reassigned afterward ininstall(), leaving a window for interleaved mutations.However, the practical risk depends on whether your application can actually trigger concurrent install calls. If installs are UI-thread-only, the risk is minimal. If background tasks or multiple threads can invoke
install(), the vulnerability is real.Recommendation: Verify whether concurrent installs are possible in your execution context. If yes, implement re-entry protection (e.g., flag guard) or switch to
@Scope("prototype")to isolate state per install invocation.
owlplug-client/src/main/java/com/owlplug/core/controllers/dialogs/AbstractDialogController.java
Show resolved
Hide resolved
owlplug-client/src/main/java/com/owlplug/core/controllers/dialogs/AbstractDialogController.java
Show resolved
Hide resolved
...lient/src/main/java/com/owlplug/explore/controllers/dialogs/InstallStepDialogController.java
Show resolved
Hide resolved
...lient/src/main/java/com/owlplug/explore/controllers/dialogs/InstallStepDialogController.java
Show resolved
Hide resolved
...lient/src/main/java/com/owlplug/explore/controllers/dialogs/InstallStepDialogController.java
Outdated
Show resolved
Hide resolved
...lient/src/main/java/com/owlplug/explore/controllers/dialogs/InstallStepDialogController.java
Show resolved
Hide resolved
...lient/src/main/java/com/owlplug/explore/controllers/dialogs/InstallStepDialogController.java
Show resolved
Hide resolved
owlplug-client/src/main/java/com/owlplug/explore/controllers/ExploreController.java
Show resolved
Hide resolved
DropSnorz
force-pushed
the
fix/plugin-install-popup
branch
from
da5d017 to
dbcd2eb
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
owlplug-client/src/main/java/com/owlplug/explore/controllers/dialogs/InstallStepDialogController.java (3)
54-54: Remove unused Swing import.
javax.swing.text.html.ImageViewis imported but never used. This is likely a leftover or mistaken import—JavaFX code usesjavafx.scene.image.ImageView.-import javax.swing.text.html.ImageView;
127-135: Consider using newValue parameter for clarity.The listener uses
obs.getValue()to check the selected toggle. Idiomatic JavaFX listener code uses then(newValue) parameter directly, making the intent clearer.- if (vst2ToggleButton.equals(obs.getValue())) { + if (vst2ToggleButton.equals(n)) { baseDir = this.getPreferences().get(ApplicationDefaults.VST_DIRECTORY_KEY, ""); - } else if (vst3ToggleButton.equals(obs.getValue())) { + } else if (vst3ToggleButton.equals(n)) { baseDir = this.getPreferences().get(ApplicationDefaults.VST3_DIRECTORY_KEY, ""); - } else if (auToggleButton.equals(obs.getValue())) { + } else if (auToggleButton.equals(n)) { baseDir = this.getPreferences().get(ApplicationDefaults.AU_DIRECTORY_KEY, ""); - } else if (lv2ToggleButton.equals(obs.getValue())) { + } else if (lv2ToggleButton.equals(n)) { baseDir = this.getPreferences().get(ApplicationDefaults.LV2_DIRECTORY_KEY, ""); }
310-317: Restore button visibility when directory is valid.When the directory is invalid, line 313 hides
directoryChooserButton, but the else block doesn't restore visibility. If the directory becomes valid later (e.g., after user changes it), the button remains hidden untilreset()is called.if (installationDirectory.exists() && !installationDirectory.isDirectory()) { directoryValidText.setText("Installation directory " + installationDirectory.getName() + " is not a directory."); directoryChooserButton.setVisible(false); result = false; } else { directoryValidText.setText("Installation directory " + installationDirectory.getName() + " is valid."); + directoryChooserButton.setVisible(true); }
📜 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/DialogManager.java(0 hunks)owlplug-client/src/main/java/com/owlplug/core/components/LazyViewRegistry.java(2 hunks)owlplug-client/src/main/java/com/owlplug/core/controllers/dialogs/AbstractDialogController.java(3 hunks)owlplug-client/src/main/java/com/owlplug/explore/controllers/ExploreController.java(3 hunks)owlplug-client/src/main/java/com/owlplug/explore/controllers/dialogs/InstallStepDialogController.java(1 hunks)owlplug-client/src/main/resources/fxml/dialogs/InstallStepView.fxml(1 hunks)
💤 Files with no reviewable changes (1)
- owlplug-client/src/main/java/com/owlplug/core/components/DialogManager.java
🚧 Files skipped from review as they are similar to previous changes (4)
- owlplug-client/src/main/resources/fxml/dialogs/InstallStepView.fxml
- owlplug-client/src/main/java/com/owlplug/core/components/LazyViewRegistry.java
- owlplug-client/src/main/java/com/owlplug/core/controllers/dialogs/AbstractDialogController.java
- owlplug-client/src/main/java/com/owlplug/explore/controllers/ExploreController.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-22T15:07:42.818Z
Learnt from: DropSnorz
PR: DropSnorz/OwlPlug#371
File: owlplug-client/src/main/java/com/owlplug/explore/controllers/dialogs/InstallStepDialogController.java:236-243
Timestamp: 2025-10-22T15:07:42.818Z
Learning: In InstallStepDialogController.java, the directoryOverrideAllowed validation is intentionally performed in verifyInstallationDirectory() before task creation, not within BundleInstallTask. The task assumes all preconditions are met because it's only created after verification succeeds.
Applied to files:
owlplug-client/src/main/java/com/owlplug/explore/controllers/dialogs/InstallStepDialogController.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 (2)
owlplug-client/src/main/java/com/owlplug/explore/controllers/dialogs/InstallStepDialogController.java (2)
352-365: Verify getRemotePackage() cannot return null in path construction.Lines 356 and 364 call
installParams.getBundle().getRemotePackage()without null checks. IfgetRemotePackage()can return null, these will throw NPE during directory path construction, breaking the installation flow.If
getRemotePackage()can be null, add guards:if (shouldGroupByCreator) { - String creator = FileUtils.sanitizeFileName(installParams.getBundle().getRemotePackage().getCreator()); - selectedDirectory = new File(selectedDirectory, creator); + if (installParams.getBundle().getRemotePackage() != null) { + String creator = FileUtils.sanitizeFileName(installParams.getBundle().getRemotePackage().getCreator()); + selectedDirectory = new File(selectedDirectory, creator); + } } boolean shouldWrapSubDirectory = this.getPreferences().getBoolean(ApplicationDefaults.STORE_SUBDIRECTORY_ENABLED, true); if (shouldWrapSubDirectory) { - String subDirectory = FileUtils.sanitizeFileName(installParams.getBundle().getRemotePackage().getName()); - selectedDirectory = new File(selectedDirectory, subDirectory); + if (installParams.getBundle().getRemotePackage() != null) { + String subDirectory = FileUtils.sanitizeFileName(installParams.getBundle().getRemotePackage().getName()); + selectedDirectory = new File(selectedDirectory, subDirectory); + } }
407-448: LGTM!The
InstallationParametersinner class is a clean POJO with appropriate default values (directoryOverrideAllowed = false,installationConfirmed = true) that align with the validation flow.
...lient/src/main/java/com/owlplug/explore/controllers/dialogs/InstallStepDialogController.java
Show resolved
Hide resolved
...lient/src/main/java/com/owlplug/explore/controllers/dialogs/InstallStepDialogController.java
Outdated
Show resolved
Hide resolved
DropSnorz
force-pushed
the
fix/plugin-install-popup
branch
from
dbcd2eb to
d269c20
Compare
Relates to #365