Implement plugin filters#490
Merged
Merged
Conversation
WalkthroughThis PR adds a filterable sidebar to the Plugins tab with checkboxes for format, type, manufacturer, and category, backed by a new reactive ChangesPlugin Filter Sidebar
Sequence Diagram(s)sequenceDiagram
participant User
participant PluginFilterController
participant PluginFilterState
participant PluginTableController
participant PluginTreeViewController
User->>PluginFilterController: toggles checkbox
PluginFilterController->>PluginFilterState: add/remove selection
PluginFilterState->>PluginFilterState: recompute predicateProperty
PluginFilterState-->>PluginTableController: filterPredicate binding fires
PluginFilterState-->>PluginTreeViewController: filterPredicate binding fires
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 4
🤖 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/plugin/components/PluginFilterModel.java`:
- Around line 36-76: The shared filter model currently ignores the
Instrument/Effect type dimension, so `PluginFilterModel` only filters by format,
manufacturer, and category. Add type selection state to the same model alongside
`selectedFormats`, `selectedManufacturers`, and `selectedCategories`, then
update `buildPredicate()` and the relevant helper logic so the returned
predicate also enforces the selected type(s) for both `PluginComponent` and
`Plugin` cases. Make sure the new type filter is wired through the existing
`predicate` binding so table and tree views stay in sync.
In
`@owlplug-client/src/main/java/com/owlplug/plugin/controllers/ComponentInfoController.java`:
- Around line 92-94: The new label in ComponentInfoController.refresh()
dereferences component.asPlugin().getFormat().getName() even when the plugin
format may be missing, which can break the details pane for unknown formats.
Update the label construction to handle a null format safely, using the same
fallback behavior already supported by PluginKindBadgeView and
PluginFormatBadgeView, and keep the refresh() path from throwing when format
information is unavailable.
In
`@owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginFilterController.java`:
- Around line 141-157: The filter counts in PluginFilterController.refresh()
only use Plugin::getManufacturerName and Plugin::getCategory, so sidebar options
miss values that exist only on PluginComponent entries. Update the manufacturer
and category aggregation logic to include metadata from both Plugin and its
components, then pass those combined counts into buildExpandableSection so every
value rendered by PluginTableController and PluginTreeViewController can be
toggled from the sidebar.
- Around line 212-223: The createCheckBox() helper is registering a new
selectedSet listener for every checkbox, but refresh() later replaces the UI
without removing those listeners, so detached CheckBox instances stay
referenced. Add listener lifecycle management so each listener is unregistered
when the checkbox is discarded, or reuse a shared listener pattern owned by
PluginFilterController instead of attaching one per checkbox. Update the
checkbox wiring in createCheckBox() and the rebuild path in refresh() to ensure
old listeners are cleaned up when the section is rebuilt.
🪄 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: 41b4a971-68db-466e-8370-ec49b8d866fb
⛔ Files ignored due to path filters (1)
owlplug-client/src/main/resources/icons/cube-white-16.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (13)
owlplug-client/src/main/java/com/owlplug/core/components/ApplicationDefaults.javaowlplug-client/src/main/java/com/owlplug/plugin/components/PluginFilterModel.javaowlplug-client/src/main/java/com/owlplug/plugin/controllers/ComponentInfoController.javaowlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginFilterController.javaowlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginInfoController.javaowlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginTableController.javaowlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginTreeViewController.javaowlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginsController.javaowlplug-client/src/main/java/com/owlplug/plugin/ui/PluginComponentCellFactory.javaowlplug-client/src/main/java/com/owlplug/plugin/ui/PluginKindBadgeView.javaowlplug-client/src/main/java/com/owlplug/plugin/ui/PluginTreeCell.javaowlplug-client/src/main/resources/fxml/plugins/PluginsView.fxmlowlplug-client/src/main/resources/owlplug.css
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginsController.java (1)
227-234: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
filterController.refresh()runs blocking repository queries on the FX Application Thread.
displayPlugins()correctly fetches plugins asynchronously viaAsync.supply, butfilterController.refresh()is invoked insideFX.run(...)at line 232, where it performs four separate JPA count queries (seePluginFilterController.refresh()). This blocks UI rendering for the duration of those queries.Move the count-fetching into the async stage and only apply the resulting UI updates inside
FX.run(...).🤖 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/plugin/controllers/PluginsController.java` around lines 227 - 234, `displayPlugins()` is still triggering blocking database work on the JavaFX Application Thread because `filterController.refresh()` is called inside `FX.run(...)`. Move the plugin count queries out of the UI callback by changing the async chain in `PluginsController.displayPlugins()` so the repository work happens in the `Async.supply`/subsequent background stage, then pass the computed filter data into `FX.run(...)` only for UI updates. Use the existing `PluginFilterController.refresh()` logic as the reference for what needs to be split so the FX thread only updates `treeViewController`, `tableController`, and the refreshed filter state.
🧹 Nitpick comments (4)
owlplug-client/src/main/java/com/owlplug/plugin/components/PluginFilterState.java (1)
53-58: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
matchesFormatbypasses the newpluginFormatfield onPluginComponent.
component.asPlugin().getFormat()reads the format through the lazily-associated parentPluginentity, whilePluginComponentnow carries its own denormalizedpluginFormatfield (set during scan specifically "based on parent plugin"). Usingcomponent.getPluginFormat()here would be consistent with the field's stated purpose and avoid relying on thepluginassociation being populated/consistent at filter-evaluation time.♻️ Proposed fix
if (plugin instanceof PluginComponent component) { - return matchesFormat(component.asPlugin().getFormat()) + return matchesFormat(component.getPluginFormat()) && matchesType(component.getType()) && matchesManufacturer(component.getManufacturerName()) && matchesCategory(component.getCategory());🤖 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/plugin/components/PluginFilterState.java` around lines 53 - 58, `PluginFilterState` is still reading the format from the parent `Plugin` via `component.asPlugin().getFormat()` instead of the denormalized `pluginFormat` stored on `PluginComponent`. Update the filter logic in the `matchesFormat` check to use `component.getPluginFormat()` so `matchesFormat` relies on the component’s own scanned data and does not depend on the association being loaded or consistent.owlplug-client/src/main/java/com/owlplug/plugin/model/PluginComponent.java (1)
56-67: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRemove the unused
PluginComponent.pluginFormatfield It only gets set during scan; the existing read paths useplugin.getFormat(), so this copy adds a second source of truth with no current consumer.🤖 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/plugin/model/PluginComponent.java` around lines 56 - 67, Remove the unused PluginComponent.pluginFormat field from PluginComponent, since the scan-time copy is not consumed anywhere and creates a duplicate source of truth. Update the model by deleting the field and its related JPA annotation, and then verify any scan or mapping code that references PluginComponent.pluginFormat is switched to use the parent Plugin’s format via plugin.getFormat() instead.owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginTableController.java (1)
98-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSearch/filter predicate logic duplicated with
PluginTreeViewController.buildTreePredicate.The combined search-match + external-predicate test (name/category substring matching, then
fp.test(plugin)) is re-implemented almost verbatim inPluginTreeViewController.buildTreePredicate(lines 93-109 there). Consider extracting a sharedPredicate<IPlugin>builder (e.g., a static helper or a method onPluginFilterState) that both controllers call, to avoid the two copies drifting apart.🤖 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/plugin/controllers/PluginTableController.java` around lines 98 - 112, The search/filter predicate logic in PluginTableController is duplicated from PluginTreeViewController.buildTreePredicate, so refactor the shared name/category search plus external predicate combination into one reusable Predicate<IPlugin> builder. Add a common helper (for example on PluginFilterState or a static utility) that takes the search value and optional fp predicate, then have both filteredPluginList.predicateProperty() and buildTreePredicate call it so the matching behavior stays in sync.owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginFilterController.java (1)
227-287: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win"View more" expansion state is lost on every rebuild.
buildExpandableSectionalways startsfullBoxcollapsed (setVisible(false)/setManaged(false)). Since this method runs on everyrefresh()(i.e., after every scan/reload), a user's previously expanded manufacturer/category list collapses back each time, which is a minor but recurring UX regression.Consider tracking the expanded state per section (e.g., a boolean field) and restoring it after rebuild.
💡 Sketch
+ private boolean manufacturerExpanded = false; + private boolean categoryExpanded = false; + private void buildExpandableSection(VBox container, Map<String, CheckBox> checkBoxMap, - Map<String, Long> countMap, Consumer<String> onToggle, ObservableSet<String> selectedSet) { + Map<String, Long> countMap, Consumer<String> onToggle, ObservableSet<String> selectedSet, + boolean expanded, Consumer<Boolean> onExpandToggle) { ... - fullBox.setVisible(false); - fullBox.setManaged(false); + fullBox.setVisible(expanded); + fullBox.setManaged(expanded); Hyperlink viewMore = new Hyperlink(...); viewMore.setOnAction(e -> { boolean newState = !fullBox.isVisible(); fullBox.setVisible(newState); fullBox.setManaged(newState); + onExpandToggle.accept(newState); ... });🤖 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/plugin/controllers/PluginFilterController.java` around lines 227 - 287, The “View more” toggle state is reset on every rebuild because buildExpandableSection always initializes the overflow VBox collapsed. Update PluginFilterController so each expandable section remembers whether it was expanded, then have buildExpandableSection restore that state after recreating the preview/full boxes; use the existing Hyperlink toggle logic and the section-specific data structures around checkBoxMap to locate the right place to persist and reapply the expanded flag.
🤖 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/plugin/controllers/PluginFilterController.java`:
- Around line 188-217: `PluginFilterController.refresh()` is doing blocking
repository work on the JavaFX thread, so split the DB queries from the UI
updates. Move the calls to
`pluginComponentRepository.countFormatsFromComponents`, `countByType`,
`countManufacturerNamesFromComponents`, and `countCategoriesFromComponents`
off-thread (or into a background task), then apply only the checkbox text
updates and `buildExpandableSection(...)` calls back on the FX thread via
`FX.run(...)`. Keep `refresh()`/its call site in
`PluginsController.displayPlugins()` responsive by ensuring no repository I/O
runs synchronously inside the FX callback.
---
Outside diff comments:
In
`@owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginsController.java`:
- Around line 227-234: `displayPlugins()` is still triggering blocking database
work on the JavaFX Application Thread because `filterController.refresh()` is
called inside `FX.run(...)`. Move the plugin count queries out of the UI
callback by changing the async chain in `PluginsController.displayPlugins()` so
the repository work happens in the `Async.supply`/subsequent background stage,
then pass the computed filter data into `FX.run(...)` only for UI updates. Use
the existing `PluginFilterController.refresh()` logic as the reference for what
needs to be split so the FX thread only updates `treeViewController`,
`tableController`, and the refreshed filter state.
---
Nitpick comments:
In
`@owlplug-client/src/main/java/com/owlplug/plugin/components/PluginFilterState.java`:
- Around line 53-58: `PluginFilterState` is still reading the format from the
parent `Plugin` via `component.asPlugin().getFormat()` instead of the
denormalized `pluginFormat` stored on `PluginComponent`. Update the filter logic
in the `matchesFormat` check to use `component.getPluginFormat()` so
`matchesFormat` relies on the component’s own scanned data and does not depend
on the association being loaded or consistent.
In
`@owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginFilterController.java`:
- Around line 227-287: The “View more” toggle state is reset on every rebuild
because buildExpandableSection always initializes the overflow VBox collapsed.
Update PluginFilterController so each expandable section remembers whether it
was expanded, then have buildExpandableSection restore that state after
recreating the preview/full boxes; use the existing Hyperlink toggle logic and
the section-specific data structures around checkBoxMap to locate the right
place to persist and reapply the expanded flag.
In
`@owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginTableController.java`:
- Around line 98-112: The search/filter predicate logic in PluginTableController
is duplicated from PluginTreeViewController.buildTreePredicate, so refactor the
shared name/category search plus external predicate combination into one
reusable Predicate<IPlugin> builder. Add a common helper (for example on
PluginFilterState or a static utility) that takes the search value and optional
fp predicate, then have both filteredPluginList.predicateProperty() and
buildTreePredicate call it so the matching behavior stays in sync.
In `@owlplug-client/src/main/java/com/owlplug/plugin/model/PluginComponent.java`:
- Around line 56-67: Remove the unused PluginComponent.pluginFormat field from
PluginComponent, since the scan-time copy is not consumed anywhere and creates a
duplicate source of truth. Update the model by deleting the field and its
related JPA annotation, and then verify any scan or mapping code that references
PluginComponent.pluginFormat is switched to use the parent Plugin’s format via
plugin.getFormat() instead.
🪄 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: 22cd1aa7-46dc-43d6-a9ad-d33a2a626124
📒 Files selected for processing (13)
CONTRIBUTORSowlplug-client/src/main/java/com/owlplug/core/utils/StringUtils.javaowlplug-client/src/main/java/com/owlplug/plugin/components/PluginFilterState.javaowlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginFilterController.javaowlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginTableController.javaowlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginTreeViewController.javaowlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginsController.javaowlplug-client/src/main/java/com/owlplug/plugin/model/PluginComponent.javaowlplug-client/src/main/java/com/owlplug/plugin/repositories/PluginComponentRepository.javaowlplug-client/src/main/java/com/owlplug/plugin/repositories/PluginRepository.javaowlplug-client/src/main/java/com/owlplug/plugin/repositories/StringCountEntry.javaowlplug-client/src/main/java/com/owlplug/plugin/tasks/PluginScanTask.javaowlplug-client/src/main/java/com/owlplug/plugin/ui/PluginComponentCellFactory.java
✅ Files skipped from review due to trivial changes (2)
- CONTRIBUTORS
- owlplug-client/src/main/java/com/owlplug/plugin/repositories/PluginRepository.java
🚧 Files skipped from review as they are similar to previous changes (1)
- owlplug-client/src/main/java/com/owlplug/plugin/ui/PluginComponentCellFactory.java
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.
Resolve #254