Offload blocking database and service calls from fx thread#466
Merged
Conversation
WalkthroughThis PR systematically refactors multiple controllers to use JavaFX ObjectProperty bindings for state management, replacing direct field references with property-driven reactive patterns. Concurrently, several blocking operations are made asynchronous using CompletableFuture, and plugin state precedence logic is adjusted to prioritize disabled status over incomplete scans. ChangesProperty-Driven Controller Refactoring
Asynchronous Operations & State Logic
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
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/controllers/DirectoryInfoController.java`:
- Around line 180-188: The repository queries in setPluginDirectory are running
on the JavaFX thread: replace the direct calls to
fileStatRepository.findByPath(path) and
fileStatRepository.findByParentPathOrderByLengthDesc(path) with an asynchronous
task (e.g. CompletableFuture.supplyAsync) and then marshal UI updates back onto
the FX thread (using FX.run() or Platform.runLater). Fetch both
Optional<FileStat> and List<FileStat> off the FX thread, then inside the FX
callback update directoryMetricsTab, directoryFilesTab, directoryFilesTableView,
pieChart and call createStatChartBuckets and pieChart.layout() so all DB work is
off the UI thread and only UI mutations happen on FX.
In
`@owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginInfoController.java`:
- Around line 143-145: The event handlers (e.g., the lambda passed to
enableButton.setOnAction that calls pluginService.enablePlugin(plugin)) close
over the mutable controller field plugin which can change before the async task
runs; capture the currently selected Plugin/PluginFootprint into a local final
variable (e.g., selectedPlugin or pluginToEnable) immediately inside the event
handler and use that local inside CompletableFuture.runAsync(...) so the
background work acts on the snapshot; apply the same fix to the other handlers
mentioned (lines 149-153) that similarly reference the field.
In
`@owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginsController.java`:
- Around line 209-213: displayPlugins can race: add a monotonically increasing
request id (e.g., an AtomicLong field like latestLoadId) that displayPlugins
increments and captures into a local variable before calling
CompletableFuture.supplyAsync(pluginRepository::findAll); in the thenAccept
handler compare the captured id with the current latestLoadId and only call
FX.run(() -> { treeViewController.setPlugins(...);
tableController.setPlugins(...); }) if they match, so stale async results (from
pluginRepository.findAll) do not overwrite newer loads.
In
`@owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginTableController.java`:
- Around line 233-235: The async call in enableItem.setOnAction uses
CompletableFuture.runAsync(() -> pluginService.enablePlugin(plugin)) but never
observes failures; attach a completion handler to log errors and update the UI
so failures do not fail silently. Replace the bare runAsync call with a chained
handler on the returned CompletableFuture (e.g., whenComplete or exceptionally)
that logs the thrown exception (using your logger) and calls Platform.runLater
to refresh the plugin table or show an error dialog; ensure you still refresh
the UI on success (call the existing refresh method) and reference
enableItem.setOnAction, pluginService.enablePlugin(plugin), and
CompletableFuture.runAsync in your changes.
🪄 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: df649f35-525b-46c1-9ab2-e491a26fee48
📒 Files selected for processing (6)
owlplug-client/src/main/java/com/owlplug/plugin/controllers/DirectoryInfoController.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/PluginsController.javaowlplug-client/src/main/java/com/owlplug/plugin/services/PluginService.javaowlplug-client/src/main/java/com/owlplug/plugin/ui/RecoveredPluginView.java
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
owlplug-client/src/main/java/com/owlplug/project/controllers/ProjectInfoController.java (1)
194-209:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
refresh()against null project values to prevent UI crashes.
refresh()assumesprojectProperty.get()is always non-null. If the property is cleared (or refresh is called before selection), this will NPE on Line 197+.💡 Suggested fix
public void refresh() { DawProject project = projectProperty.get(); + if (project == null) { + projectInfoPane.setVisible(false); + projectOpenButton.setDisable(true); + pluginTable.setItems(FXCollections.observableArrayList()); + return; + } projectInfoPane.setVisible(true); projectNameLabel.setText(project.getName()); projectAppLabel.setText(project.getApplication().getName()); projectAppImageView.setImage(this.getApplicationDefaults().getDAWApplicationIcon(project.getApplication())); projectOpenButton.setDisable(false);🤖 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/project/controllers/ProjectInfoController.java` around lines 194 - 209, The refresh() method assumes projectProperty.get() is non-null; add a null check at the start of refresh() to safely handle cleared/no-selection cases: if projectProperty.get() is null, hide or clear the UI (e.g., projectInfoPane.setVisible(false)), disable/clear controls (projectOpenButton.setDisable(true), clear labels like projectNameLabel, projectAppLabel, appFullNameLabel, projectCreatedLabel, projectLastModifiedLabel, projectPluginsFoundLabel, projectFormatVersionLabel, projectPathLabel) and set pluginTable.setItems(FXCollections.emptyObservableList()), then return; otherwise proceed with the existing code that reads project fields. Ensure you reference projectProperty, refresh(), projectInfoPane, projectOpenButton, all label fields and pluginTable when implementing the guard.owlplug-client/src/main/java/com/owlplug/plugin/controllers/DirectoryInfoController.java (1)
174-201:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a null guard in
refresh()(consistent withPluginInfoController.refresh()).
pluginDirectoryProperty.get()is dereferenced on line 176 without a null check. If the property is ever reset tonull, the listener triggers and this NPEs. The pattern inPluginInfoController.refresh()(early return on null) is worth mirroring here for safety.🛡️ Proposed guard
public void refresh() { PluginDirectory pluginDirectory = pluginDirectoryProperty.get(); + if (pluginDirectory == null) { + return; + } directoryPathTextField.setText(pluginDirectory.getPath());🤖 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/DirectoryInfoController.java` around lines 174 - 201, In DirectoryInfoController.refresh(), guard against pluginDirectoryProperty being null before dereferencing: fetch PluginDirectory via pluginDirectoryProperty.get(), and if it is null return early (same pattern used in PluginInfoController.refresh()); update subsequent usage of pluginDirectory (getPath/getName/getPluginList) to assume non-null after this check so the method doesn't NPE when the property is reset.owlplug-client/src/main/java/com/owlplug/plugin/controllers/ComponentInfoController.java (1)
89-103:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a null guard in
refresh()(consistent withPluginInfoController.refresh()).
componentProperty.get()is dereferenced on line 91 without a null check. If the property is reset tonull, the listener fires and the body NPEs. Mirror the early-return pattern fromPluginInfoController.refresh()here.🛡️ Proposed guard
public void refresh() { PluginComponent component = componentProperty.get(); + if (component == null) { + return; + } pluginFormatIcon.setImage(this.getApplicationDefaults().getPluginFormatIcon(component.getPlugin().getFormat()));🤖 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/ComponentInfoController.java` around lines 89 - 103, The refresh() method dereferences componentProperty.get() without a null guard which can NPE when the property is reset; add the same early-return null check used in PluginInfoController.refresh(): get the PluginComponent via componentProperty.get(), if it is null return immediately (or clear/reset the UI as PluginInfoController does) before accessing component, so all subsequent uses (pluginFormatIcon, pluginFormatLabel, pluginTitleLabel, etc.) are protected.owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginInfoController.java (1)
207-218:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMove
tryResolveAndSaveImageUrl()off the FX thread using async pattern.
setPluginImage()runs on the FX thread (invoked fromrefresh()which is called viaFX.run()at line 274). At line 215,pluginService.tryResolveAndSaveImageUrl(plugin)performs blocking operations:resolveImageUrl()queries the remote package repository, andpluginFootprintRepository.save()writes to the database. Line 217 immediately callsgetScreenshotUrl(), relying on the side-effect from line 215 to have completed synchronously, blocking the UI thread.Use
CompletableFuture.runAsync()to perform the resolve/save off-thread, then reapply the image viaFX.run()once complete. This matches the async pattern already used elsewhere in this controller (lines 150, 159).🤖 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/PluginInfoController.java` around lines 207 - 218, setPluginImage() is performing blocking resolve/save work on the JavaFX thread by calling pluginService.tryResolveAndSaveImageUrl(plugin) and then immediately re-reading plugin.getFootprint().getScreenshotUrl(); move the blocking call off the FX thread using CompletableFuture.runAsync(() -> pluginService.tryResolveAndSaveImageUrl(plugin)) and, when that completes, re-read the footprint screenshot URL and update the UI inside FX.run(() -> { ... }) so setPluginImage() only triggers async work and UI updates occur on the FX thread; ensure you still handle null/empty checks on plugin.getScreenshotUrl() and plugin.getFootprint().getScreenshotUrl() and that the final getScreenshotUrl() used to set the image is obtained after the async completion.
🤖 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/PluginInfoController.java`:
- Around line 148-161: The async fire-and-forget calls in
enableButton.setOnAction (which calls pluginService.enablePlugin) and the
nativeDiscoveryToggleButton listener (which calls pluginService.save on
plugin.getFootprint()) must handle exceptions and surface them to the UI: change
CompletableFuture.runAsync(...) to attach whenComplete/exceptionally handlers
that route errors to the existing notification/dialog plumbing and log them; for
the nativeDiscoveryToggleButton listener also revert the toggle state on failure
via Platform.runLater using pluginProperty.get() to restore the previous value
so the visible state matches the persisted state.
In
`@owlplug-client/src/main/java/com/owlplug/plugin/controllers/SymlinkInfoController.java`:
- Around line 108-114: The refresh() method dereferences symlinkProperty.get()
without a null check; mirror PluginInfoController behavior by early-returning if
symlinkProperty.get() == null to avoid NPEs. In refresh(), fetch Symlink symlink
= symlinkProperty.get(); if symlink is null return immediately; otherwise
continue to set directoryPathLabel,
pluginDirectoryListView.getItems().setAll(symlink.getPluginList()),
targetPathLabel (using Optional.ofNullable...), and
openTargetButton.setVisible(...) so behavior remains identical when a property
is unset.
---
Outside diff comments:
In
`@owlplug-client/src/main/java/com/owlplug/plugin/controllers/ComponentInfoController.java`:
- Around line 89-103: The refresh() method dereferences componentProperty.get()
without a null guard which can NPE when the property is reset; add the same
early-return null check used in PluginInfoController.refresh(): get the
PluginComponent via componentProperty.get(), if it is null return immediately
(or clear/reset the UI as PluginInfoController does) before accessing component,
so all subsequent uses (pluginFormatIcon, pluginFormatLabel, pluginTitleLabel,
etc.) are protected.
In
`@owlplug-client/src/main/java/com/owlplug/plugin/controllers/DirectoryInfoController.java`:
- Around line 174-201: In DirectoryInfoController.refresh(), guard against
pluginDirectoryProperty being null before dereferencing: fetch PluginDirectory
via pluginDirectoryProperty.get(), and if it is null return early (same pattern
used in PluginInfoController.refresh()); update subsequent usage of
pluginDirectory (getPath/getName/getPluginList) to assume non-null after this
check so the method doesn't NPE when the property is reset.
In
`@owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginInfoController.java`:
- Around line 207-218: setPluginImage() is performing blocking resolve/save work
on the JavaFX thread by calling pluginService.tryResolveAndSaveImageUrl(plugin)
and then immediately re-reading plugin.getFootprint().getScreenshotUrl(); move
the blocking call off the FX thread using CompletableFuture.runAsync(() ->
pluginService.tryResolveAndSaveImageUrl(plugin)) and, when that completes,
re-read the footprint screenshot URL and update the UI inside FX.run(() -> { ...
}) so setPluginImage() only triggers async work and UI updates occur on the FX
thread; ensure you still handle null/empty checks on plugin.getScreenshotUrl()
and plugin.getFootprint().getScreenshotUrl() and that the final
getScreenshotUrl() used to set the image is obtained after the async completion.
In
`@owlplug-client/src/main/java/com/owlplug/project/controllers/ProjectInfoController.java`:
- Around line 194-209: The refresh() method assumes projectProperty.get() is
non-null; add a null check at the start of refresh() to safely handle
cleared/no-selection cases: if projectProperty.get() is null, hide or clear the
UI (e.g., projectInfoPane.setVisible(false)), disable/clear controls
(projectOpenButton.setDisable(true), clear labels like projectNameLabel,
projectAppLabel, appFullNameLabel, projectCreatedLabel,
projectLastModifiedLabel, projectPluginsFoundLabel, projectFormatVersionLabel,
projectPathLabel) and set
pluginTable.setItems(FXCollections.emptyObservableList()), then return;
otherwise proceed with the existing code that reads project fields. Ensure you
reference projectProperty, refresh(), projectInfoPane, projectOpenButton, all
label fields and pluginTable when implementing the guard.
🪄 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: 5a922c88-ace1-4038-9068-7aa5fbd507ac
📒 Files selected for processing (7)
owlplug-client/src/main/java/com/owlplug/plugin/controllers/ComponentInfoController.javaowlplug-client/src/main/java/com/owlplug/plugin/controllers/DirectoryInfoController.javaowlplug-client/src/main/java/com/owlplug/plugin/controllers/NodeInfoController.javaowlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginInfoController.javaowlplug-client/src/main/java/com/owlplug/plugin/controllers/SymlinkInfoController.javaowlplug-client/src/main/java/com/owlplug/project/controllers/ProjectInfoController.javaowlplug-client/src/main/java/com/owlplug/project/controllers/ProjectsController.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.
No description provided.