Refactor options view display and add custom theme provider#481
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughIntroduces a custom ChangesOptions UI Refactor + Custom Dark Theme
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
owlplug-client/src/main/resources/fxml/options/PluginScanOptionsView.fxml (2)
29-36: ⚡ Quick winEditable Spinner may allow out-of-range values.
The
loaderTimeoutSpinneris markededitable="true", allowing users to type values directly. Without additional validation, users could enter values outside the 0-3600 range defined in theSpinnerValueFactory. Consider either removingeditable="true"or adding aTextFormatterto enforce range constraints.🤖 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 `@owlplug-client/src/main/resources/fxml/options/PluginScanOptionsView.fxml` around lines 29 - 36, The loaderTimeoutSpinner element is marked as editable="true", which allows users to type values directly and bypass the range validation defined in the SpinnerValueFactory. Remove the editable="true" attribute from the loaderTimeoutSpinner Spinner element to prevent direct text input and ensure users can only select values within the valid 0-3600 second range using the spinner controls.
30-31: 💤 Low valueGrammatical issue: checkbox label implies continuation.
The label "Enable native plugin discovery using" grammatically expects the ComboBox value to complete the sentence. If no loader is selected initially, the sentence is incomplete. Consider rephrasing to "Enable native plugin discovery" and adding a separate Label like "Loader:" before the ComboBox for clarity.
🤖 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 `@owlplug-client/src/main/resources/fxml/options/PluginScanOptionsView.fxml` around lines 30 - 31, The CheckBox with fx:id pluginNativeCheckbox has a label "Enable native plugin discovery using" that grammatically expects completion by the ComboBox value, making it incomplete when no loader is selected. Rephrase the checkbox label text to "Enable native plugin discovery" by removing "using", and add a new Label element before the pluginNativeComboBox with text like "Loader:" to provide clear context for what the ComboBox represents without relying on grammatical continuation.owlplug-client/src/main/java/com/owlplug/core/controllers/OptionsController.java (1)
141-143: ⚡ Quick winConsider adding bounds validation.
navigateToSection(int index)does not validate the index before accessingnavToggleGroup.getToggles().get(index), which could throwIndexOutOfBoundsExceptionif called with an invalid index. Since this is a public method, consider adding bounds checking.🛡️ Proposed fix
public void navigateToSection(int index) { + if (index < 0 || index >= navToggleGroup.getToggles().size()) { + throw new IllegalArgumentException("Invalid section index: " + index); + } navToggleGroup.getToggles().get(index).setSelected(true); }🤖 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 `@owlplug-client/src/main/java/com/owlplug/core/controllers/OptionsController.java` around lines 141 - 143, The navigateToSection(int index) method accesses navToggleGroup.getToggles().get(index) without validating that the index is within the bounds of the list, which could throw IndexOutOfBoundsException. Add bounds validation before accessing the list to ensure the index is greater than or equal to 0 and less than the size of the toggles collection. If the index is invalid, either return early from the method or throw an IllegalArgumentException with a descriptive message. Only call setSelected(true) on the toggle if the index has been validated as valid.owlplug-client/src/main/java/com/owlplug/core/controllers/MainController.java (1)
134-134: ⚡ Quick winReplace magic number with constant.
The hardcoded
2should be replaced withEXPLORE_TAB_INDEXfor maintainability.♻️ Proposed fix
- if (newValue.intValue() == 2) { + if (newValue.intValue() == EXPLORE_TAB_INDEX) { exploreController.requestLayout(); }🤖 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 `@owlplug-client/src/main/java/com/owlplug/core/controllers/MainController.java` at line 134, Replace the hardcoded magic number 2 in the condition at line 134 of the MainController.java file with the EXPLORE_TAB_INDEX constant. Change the comparison from intValue() == 2 to intValue() == EXPLORE_TAB_INDEX to improve code maintainability and make the intent of the comparison clearer.
🤖 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
`@owlplug-client/src/main/java/com/owlplug/core/controllers/dialogs/ListDirectoryDialogController.java`:
- Around line 97-109: The configure() method in ListDirectoryDialogController
attaches a ListChangeListener to observableItems every time it is called without
removing the previously attached listener. If configure() is invoked multiple
times, listeners accumulate on the same observable list, causing the preference
persistence and event publishing logic (the code that calls
getPreferences().putList() and publisher.publishEvent()) to execute multiple
times per list change. Store the ListChangeListener as a field variable and
remove the old listener before attaching a new one in the configure() method to
ensure only a single listener is active.
In
`@owlplug-client/src/main/java/com/owlplug/core/controllers/options/InstallationOptionsController.java`:
- Around line 114-126: The simulatePath method contains a path traversal
vulnerability where user input from storeDirectoryTextField is appended without
sanitization. Before appending the sub variable to the path via new File(path,
sub), validate that the subdirectory input does not contain path traversal
sequences like ".." or absolute path components that could escape the intended
base directory. Implement a sanitization function that normalizes the path and
verifies the resolved path stays within the base directory, or use a whitelist
approach to only allow safe directory names. This validation should occur before
constructing the File object at the line where storeDirectoryTextField input is
used.
In
`@owlplug-client/src/main/java/com/owlplug/core/controllers/options/PluginScanOptionsController.java`:
- Around line 106-111: The loaderTimeoutSpinner currently allows a minimum
timeout value of 0 seconds, which is impractical and does not align with the
default of 10 seconds. Either change the minimum constraint in the
SpinnerValueFactory.IntegerSpinnerValueFactory constructor from 0 to a practical
minimum like 1 (or 5) seconds, or add validation in the setScannerTimeout()
method to reject zero or negative timeout values before processing them. The
former approach (adjusting the spinner's lower bound) is simpler and prevents
invalid values at the UI level.
- Around line 131-136: Add a defensive null check in PluginScanTask before
calling getCurrentPluginLoader().isAvailable() to ensure consistency with the
defensive null checking pattern used in NativeHostService.loadPlugin(). Even
though initialization logic guarantees getCurrentPluginLoader() returns a
non-null DummyPluginLoader fallback, adding the null check maintains consistency
across the codebase and improves defensive programming practices.
In `@owlplug-theme/src/main/java/com/owlplug/theme/OwlPlugDarkTheme.java`:
- Around line 33-42: The getUserAgentStylesheetBSS() method attempts to load a
resource "owlplug-dark-bss.css" that does not exist, causing a
NullPointerException at runtime. Either create the missing SCSS source file
"owlplug-dark-bss.scss" in the appropriate source directory and add a
corresponding compilation rule in pom.xml (similar to the existing rule for
"owlplug-dark.scss" at line 84) to generate the CSS file during the build, or
remove the getUserAgentStylesheetBSS() method entirely if it is not required by
the implementation.
---
Nitpick comments:
In
`@owlplug-client/src/main/java/com/owlplug/core/controllers/MainController.java`:
- Line 134: Replace the hardcoded magic number 2 in the condition at line 134 of
the MainController.java file with the EXPLORE_TAB_INDEX constant. Change the
comparison from intValue() == 2 to intValue() == EXPLORE_TAB_INDEX to improve
code maintainability and make the intent of the comparison clearer.
In
`@owlplug-client/src/main/java/com/owlplug/core/controllers/OptionsController.java`:
- Around line 141-143: The navigateToSection(int index) method accesses
navToggleGroup.getToggles().get(index) without validating that the index is
within the bounds of the list, which could throw IndexOutOfBoundsException. Add
bounds validation before accessing the list to ensure the index is greater than
or equal to 0 and less than the size of the toggles collection. If the index is
invalid, either return early from the method or throw an
IllegalArgumentException with a descriptive message. Only call setSelected(true)
on the toggle if the index has been validated as valid.
In `@owlplug-client/src/main/resources/fxml/options/PluginScanOptionsView.fxml`:
- Around line 29-36: The loaderTimeoutSpinner element is marked as
editable="true", which allows users to type values directly and bypass the range
validation defined in the SpinnerValueFactory. Remove the editable="true"
attribute from the loaderTimeoutSpinner Spinner element to prevent direct text
input and ensure users can only select values within the valid 0-3600 second
range using the spinner controls.
- Around line 30-31: The CheckBox with fx:id pluginNativeCheckbox has a label
"Enable native plugin discovery using" that grammatically expects completion by
the ComboBox value, making it incomplete when no loader is selected. Rephrase
the checkbox label text to "Enable native plugin discovery" by removing "using",
and add a new Label element before the pluginNativeComboBox with text like
"Loader:" to provide clear context for what the ComboBox represents without
relying on grammatical continuation.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ecf5cd34-56ea-4866-b619-b0c5cdb47314
⛔ Files ignored due to path filters (4)
owlplug-client/pom.xmlis excluded by!**/*.xmlowlplug-client/src/main/resources/icons/heart-white-32.pngis excluded by!**/*.png,!**/*.pngowlplug-theme/pom.xmlis excluded by!**/*.xmlpom.xmlis excluded by!**/*.xml
📒 Files selected for processing (27)
owlplug-client/src/main/java/com/owlplug/OwlPlug.javaowlplug-client/src/main/java/com/owlplug/OwlPlugPreloader.javaowlplug-client/src/main/java/com/owlplug/core/controllers/MainController.javaowlplug-client/src/main/java/com/owlplug/core/controllers/OptionsController.javaowlplug-client/src/main/java/com/owlplug/core/controllers/dialogs/ListDirectoryDialogController.javaowlplug-client/src/main/java/com/owlplug/core/controllers/dialogs/WelcomeDialogController.javaowlplug-client/src/main/java/com/owlplug/core/controllers/fragments/PluginPathFragmentController.javaowlplug-client/src/main/java/com/owlplug/core/controllers/options/ApplicationOptionsController.javaowlplug-client/src/main/java/com/owlplug/core/controllers/options/InstallationOptionsController.javaowlplug-client/src/main/java/com/owlplug/core/controllers/options/PluginScanOptionsController.javaowlplug-client/src/main/java/com/owlplug/core/controllers/options/ProjectsOptionsController.javaowlplug-client/src/main/java/com/owlplug/plugin/model/PluginFormat.javaowlplug-client/src/main/java/com/owlplug/project/controllers/ProjectInfoController.javaowlplug-client/src/main/java/com/owlplug/project/controllers/ProjectsController.javaowlplug-client/src/main/resources/fxml/MainView.fxmlowlplug-client/src/main/resources/fxml/OptionsView.fxmlowlplug-client/src/main/resources/fxml/dialogs/ListDirectoryView.fxmlowlplug-client/src/main/resources/fxml/fragments/PluginPathFragment.fxmlowlplug-client/src/main/resources/fxml/options/ApplicationOptionsView.fxmlowlplug-client/src/main/resources/fxml/options/InstallationOptionsView.fxmlowlplug-client/src/main/resources/fxml/options/PluginScanOptionsView.fxmlowlplug-client/src/main/resources/fxml/options/ProjectsOptionsView.fxmlowlplug-client/src/main/resources/fxml/projects/ProjectsView.fxmlowlplug-client/src/main/resources/owlplug.cssowlplug-theme/.gitignoreowlplug-theme/src/main/java/com/owlplug/theme/OwlPlugDarkTheme.javaowlplug-theme/src/main/scss/owlplug-dark.scss
💤 Files with no reviewable changes (1)
- owlplug-client/src/main/resources/fxml/MainView.fxml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.