Refactor controller injection and add event publisher#449
Merged
Conversation
WalkthroughRefactors controller-to-controller calls into a Spring event-driven model, adds lightweight event types, centralizes JavaFX thread scheduling via a new FX utility, relocates ListDirectoryDialogController to core, and replaces direct UI refreshes with published events and event listeners. Changes
Sequence Diagram(s)sequenceDiagram
participant Service as Service/Controller
participant AEP as ApplicationEventPublisher
participant Listener as EventListener
participant FX as FX.run()
participant UI as UI Component
Service->>AEP: publishEvent(new Event(...))
AEP->>Listener: invoke `@EventListener` method
Listener->>FX: FX.run(task)
FX->>FX: if (on FX Thread) else schedule via Platform.runLater()
FX->>UI: execute task to update UI
UI->>UI: refresh/update view/state
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
✅ 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: 7
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/auth/services/AuthenticationService.java (1)
153-159:⚠️ Potential issue | 🟠 MajorPublish
AccountUpdateEventafter transaction commit.Event at line 158 is fired inside a
@Transactionalmethod with a listener that refreshes account data. Default@EventListenercan execute before transaction commit, causing the UI to observe stale state. Ensure event delivery is synchronized with transaction completion usingTransactionSynchronizationManageror by switching the listener to@TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT). Line 119 has the same pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@owlplug-client/src/main/java/com/owlplug/auth/services/AuthenticationService.java` around lines 153 - 159, The AccountUpdateEvent is being published inside the transactional method deleteAccount (and similarly in the other transactional method around line 119), which may notify listeners before the transaction commits; change the publish to occur after commit by either: (a) registering a synchronization via TransactionSynchronizationManager.registerSynchronization(...) inside deleteAccount to publish the AccountUpdateEvent in afterCommit(), or (b) leave the publishEvent call as-is but update the listener to use `@TransactionalEventListener`(phase = TransactionPhase.AFTER_COMMIT); reference the deleteAccount method and the AccountUpdateEvent and pick one approach to ensure delivery occurs only after transaction commit.
🧹 Nitpick comments (2)
owlplug-client/src/main/java/com/owlplug/core/events/PreferencesChangedEvent.java (1)
21-22: Consider using a record for consistency with other event types.
RemoteSourceUpdatedEventin this PR uses a record. For consistency across the event types, consider converting this to a record as well. If specific preference context becomes useful later, you could add an optional field.♻️ Suggested refactor
-public class PreferencesChangedEvent { -} +public record PreferencesChangedEvent() { +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@owlplug-client/src/main/java/com/owlplug/core/events/PreferencesChangedEvent.java` around lines 21 - 22, PreferencesChangedEvent is currently an empty class but other event types (e.g., RemoteSourceUpdatedEvent) are records; convert PreferencesChangedEvent to a record named PreferencesChangedEvent to match the style and make it concise, optionally adding a field like Map<String,Object> preferences or Optional<Preferences> if you want to carry context later; update any places that instantiate or reference the class to use the record constructor and accessors.owlplug-client/src/main/java/com/owlplug/core/controllers/MainController.java (1)
190-191: PublishPreferencesChangedEventonly on an actual state change.Lines 190-191 currently broadcast on every startup, even when
FIRST_LAUNCH_KEYis alreadyfalse, which causes unnecessary refresh fan-out.💡 Proposed refactor
- this.getPreferences().putBoolean(ApplicationDefaults.FIRST_LAUNCH_KEY, false); - publisher.publishEvent(new PreferencesChangedEvent()); + boolean firstLaunch = this.getPreferences().getBoolean(ApplicationDefaults.FIRST_LAUNCH_KEY, true); + if (firstLaunch) { + this.getPreferences().putBoolean(ApplicationDefaults.FIRST_LAUNCH_KEY, false); + publisher.publishEvent(new PreferencesChangedEvent()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@owlplug-client/src/main/java/com/owlplug/core/controllers/MainController.java` around lines 190 - 191, Currently the code unconditionally sets FIRST_LAUNCH_KEY to false and always calls publisher.publishEvent(new PreferencesChangedEvent()), causing spurious events; modify MainController so you first read the current value via this.getPreferences().getBoolean(ApplicationDefaults.FIRST_LAUNCH_KEY, true), compare it to the intended false state, only call this.getPreferences().putBoolean(ApplicationDefaults.FIRST_LAUNCH_KEY, false) and publisher.publishEvent(new PreferencesChangedEvent()) when the previous value was true (i.e., an actual change occurred), thus avoiding emitting PreferencesChangedEvent when no state change happened.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@owlplug-client/src/main/java/com/owlplug/core/components/TaskRunner.java`:
- Around line 200-202: In TaskRunner.registerTaskBarController, add fail-fast
validation: check the incoming tbc is not null and throw an
IllegalArgumentException (or NullPointerException) if it is, and prevent
accidental rebinding by throwing an IllegalStateException when
this.taskBarController is already set to a different instance; only assign tbc
to taskBarController after these checks (referencing registerTaskBarController,
TaskRunner, and taskBarController).
In
`@owlplug-client/src/main/java/com/owlplug/core/controllers/dialogs/WelcomeDialogController.java`:
- Around line 129-132: The PreferencesChangedEvent listener
handle(PreferencesChangedEvent) may call refreshView() before initialize() sets
up fragment controllers, causing NPEs; add a guard (e.g., a private boolean
initialized flag set to true at the end of initialize() or check that the
fragment controller fields used by refreshView are non-null) and change
handle(...) to only call FX.run(this::refreshView) when initialized is true (or
when those fragment controller fields are non-null) so the event-driven refresh
is skipped until the controller is fully initialized.
In
`@owlplug-client/src/main/java/com/owlplug/explore/controllers/ExploreController.java`:
- Around line 412-415: The handler handle(RemoteSourceUpdatedEvent) currently
calls FX.run(this::refreshView) directly and can start overlapping searches; fix
this by debouncing/coalescing events: introduce a single-thread
ScheduledExecutorService (e.g., scheduledExecutor) and a field
ScheduledFuture<?> pendingRefresh, and in handle(...) cancel pendingRefresh if
non-null then schedule a single task (with short delay, e.g., 100-300ms) that
invokes FX.run(this::refreshView); this ensures bursty RemoteSourceUpdatedEvent
events are coalesced into one refresh and prevents concurrent refreshView
executions.
In
`@owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginsController.java`:
- Around line 242-245: handle(...) currently calls displayPlugins() on the FX
thread, but displayPlugins() invokes pluginRepository.findAll() which will block
rendering; change handle(PluginUpdateEvent) to kick off a background fetch
(e.g., CompletableFuture.supplyAsync or an Executor) that calls
pluginRepository.findAll(), then on completion call FX.run(...) to pass the
resulting list to a UI-only method (rename or overload displayPlugins to accept
the fetched List or add updatePluginsUI(List) that only updates controls).
Ensure no direct calls to pluginRepository.findAll() occur inside FX.run and
keep all DB/IO on the background thread and all UI mutation inside FX.run.
In
`@owlplug-client/src/main/java/com/owlplug/plugin/events/PluginRefreshEvent.java`:
- Line 22: Fix the Javadoc grammar typo in the class comment for
PluginRefreshEvent: change the phrase "Plugin data as been updated in database."
to "Plugin data has been updated in database." by editing the Javadoc above the
PluginRefreshEvent class (ensure the corrected text appears in the Javadoc block
for the PluginRefreshEvent class or its file-level comment).
In
`@owlplug-client/src/main/java/com/owlplug/plugin/events/PluginUpdateEvent.java`:
- Around line 21-25: Fix the Javadoc typo in PluginUpdateEvent by changing "as
been" to "has been" and convert the empty class PluginUpdateEvent into a record
(e.g., public record PluginUpdateEvent(...) {}) to match the style used by
RemoteSourceUpdatedEvent; ensure any fields, constructors, or methods required
for event payload are moved into the record declaration and update usages to the
record's canonical constructor/accessors (refer to PluginUpdateEvent and
RemoteSourceUpdatedEvent to mirror field names and API).
In `@owlplug-client/src/main/resources/application.properties`:
- Line 19: Project fails startup due to unresolved circular dependency:
ProjectInfoController and SourceMenuController still inject MainController
directly (unlike ExploreController which uses `@Lazy`), causing
BeanCurrentlyInCreationException with circular references disabled; fix by
either annotating the MainController injection points in ProjectInfoController
and SourceMenuController with `@Lazy` to break the cycle (i.e., add `@Lazy` on the
field/constructor parameter where MainController is referenced), or complete the
refactor by removing direct MainController injections from ProjectInfoController
and SourceMenuController and replace them with application event
listeners/publishers so MainController is not directly referenced (ensure
listener methods are annotated with `@EventListener` and events are published via
ApplicationEventPublisher).
---
Outside diff comments:
In
`@owlplug-client/src/main/java/com/owlplug/auth/services/AuthenticationService.java`:
- Around line 153-159: The AccountUpdateEvent is being published inside the
transactional method deleteAccount (and similarly in the other transactional
method around line 119), which may notify listeners before the transaction
commits; change the publish to occur after commit by either: (a) registering a
synchronization via
TransactionSynchronizationManager.registerSynchronization(...) inside
deleteAccount to publish the AccountUpdateEvent in afterCommit(), or (b) leave
the publishEvent call as-is but update the listener to use
`@TransactionalEventListener`(phase = TransactionPhase.AFTER_COMMIT); reference
the deleteAccount method and the AccountUpdateEvent and pick one approach to
ensure delivery occurs only after transaction commit.
---
Nitpick comments:
In
`@owlplug-client/src/main/java/com/owlplug/core/controllers/MainController.java`:
- Around line 190-191: Currently the code unconditionally sets FIRST_LAUNCH_KEY
to false and always calls publisher.publishEvent(new PreferencesChangedEvent()),
causing spurious events; modify MainController so you first read the current
value via this.getPreferences().getBoolean(ApplicationDefaults.FIRST_LAUNCH_KEY,
true), compare it to the intended false state, only call
this.getPreferences().putBoolean(ApplicationDefaults.FIRST_LAUNCH_KEY, false)
and publisher.publishEvent(new PreferencesChangedEvent()) when the previous
value was true (i.e., an actual change occurred), thus avoiding emitting
PreferencesChangedEvent when no state change happened.
In
`@owlplug-client/src/main/java/com/owlplug/core/events/PreferencesChangedEvent.java`:
- Around line 21-22: PreferencesChangedEvent is currently an empty class but
other event types (e.g., RemoteSourceUpdatedEvent) are records; convert
PreferencesChangedEvent to a record named PreferencesChangedEvent to match the
style and make it concise, optionally adding a field like Map<String,Object>
preferences or Optional<Preferences> if you want to carry context later; update
any places that instantiate or reference the class to use the record constructor
and accessors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5df13902-fc12-4442-9ae0-60388136ccc8
📒 Files selected for processing (50)
owlplug-client/src/main/java/com/owlplug/OwlPlug.javaowlplug-client/src/main/java/com/owlplug/auth/controllers/AccountController.javaowlplug-client/src/main/java/com/owlplug/auth/events/AccountUpdateEvent.javaowlplug-client/src/main/java/com/owlplug/auth/services/AuthenticationService.javaowlplug-client/src/main/java/com/owlplug/core/components/DialogManager.javaowlplug-client/src/main/java/com/owlplug/core/components/TaskRunner.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/TaskBarController.javaowlplug-client/src/main/java/com/owlplug/core/controllers/dialogs/CrashRecoveryDialogController.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/events/PreferencesChangedEvent.javaowlplug-client/src/main/java/com/owlplug/core/services/AppUpdateService.javaowlplug-client/src/main/java/com/owlplug/core/services/BaseService.javaowlplug-client/src/main/java/com/owlplug/core/services/OptionsService.javaowlplug-client/src/main/java/com/owlplug/core/services/TelemetryService.javaowlplug-client/src/main/java/com/owlplug/core/utils/FX.javaowlplug-client/src/main/java/com/owlplug/explore/controllers/ExploreController.javaowlplug-client/src/main/java/com/owlplug/explore/controllers/NewSourceDialogController.javaowlplug-client/src/main/java/com/owlplug/explore/controllers/PackageInfoController.javaowlplug-client/src/main/java/com/owlplug/explore/controllers/SourceMenuController.javaowlplug-client/src/main/java/com/owlplug/explore/controllers/dialogs/InstallStepDialogController.javaowlplug-client/src/main/java/com/owlplug/explore/events/RemoteSourceUpdatedEvent.javaowlplug-client/src/main/java/com/owlplug/explore/services/ExploreService.javaowlplug-client/src/main/java/com/owlplug/plugin/components/PluginTaskFactory.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/controllers/dialogs/DisablePluginDialogController.javaowlplug-client/src/main/java/com/owlplug/plugin/events/PluginRefreshEvent.javaowlplug-client/src/main/java/com/owlplug/plugin/events/PluginUpdateEvent.javaowlplug-client/src/main/java/com/owlplug/plugin/services/NativeHostService.javaowlplug-client/src/main/java/com/owlplug/plugin/services/PluginService.javaowlplug-client/src/main/java/com/owlplug/project/controllers/ProjectsController.javaowlplug-client/src/main/java/com/owlplug/project/services/PluginLookupService.javaowlplug-client/src/main/resources/application.propertiesowlplug-client/src/main/resources/fxml/MainView.fxmlowlplug-client/src/main/resources/fxml/dialogs/ListDirectoryView.fxmlowlplug-client/src/main/resources/fxml/explore/ExploreView.fxmlowlplug-client/src/main/resources/fxml/explore/PackageInfoView.fxmlowlplug-client/src/main/resources/fxml/plugins/ComponentInfoView.fxmlowlplug-client/src/main/resources/fxml/plugins/DirectoryInfoView.fxmlowlplug-client/src/main/resources/fxml/plugins/NodeInfoView.fxmlowlplug-client/src/main/resources/fxml/plugins/PluginInfoView.fxmlowlplug-client/src/main/resources/fxml/plugins/PluginsView.fxmlowlplug-client/src/main/resources/fxml/plugins/SymlinkInfoView.fxmlowlplug-client/src/main/resources/fxml/projects/ProjectInfoView.fxmlowlplug-client/src/main/resources/fxml/projects/ProjectsView.fxml
💤 Files with no reviewable changes (6)
- owlplug-client/src/main/java/com/owlplug/plugin/controllers/dialogs/DisablePluginDialogController.java
- owlplug-client/src/main/java/com/owlplug/explore/controllers/NewSourceDialogController.java
- owlplug-client/src/main/java/com/owlplug/plugin/services/NativeHostService.java
- owlplug-client/src/main/java/com/owlplug/auth/controllers/AccountController.java
- owlplug-client/src/main/java/com/owlplug/core/services/AppUpdateService.java
- owlplug-client/src/main/java/com/owlplug/core/services/TelemetryService.java
DropSnorz
force-pushed
the
chore/refactor-controller-di
branch
from
d123793 to
c83b1bf
Compare
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
owlplug-client/src/main/java/com/owlplug/plugin/services/PluginService.java (2)
148-167:⚠️ Potential issue | 🟡 MinorSame pattern: event published after failure path in
enablePlugin.Similar to
disablePlugin, thePluginRefreshEventat line 166 fires even when the rename operation fails.💡 Consistent fix
if (originFile.renameTo(destFile)) { plugin.setDisabled(false); plugin.setPath(newPath); pluginRepository.save(plugin); + publisher.publishEvent(new PluginRefreshEvent()); } else { log.error("Plugin can't be enabled: failed to rename file {}", plugin.getPath()); } - - publisher.publishEvent(new PluginRefreshEvent());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@owlplug-client/src/main/java/com/owlplug/plugin/services/PluginService.java` around lines 148 - 167, The PluginRefreshEvent is published regardless of whether the rename succeeded in enablePlugin; move the publisher.publishEvent(new PluginRefreshEvent()) so it only runs after a successful rename (i.e., inside the if (originFile.renameTo(destFile)) block where plugin.setDisabled(false), plugin.setPath(newPath), and pluginRepository.save(plugin) are executed); keep the error log in the else branch unchanged so no event is emitted on failure and the success branch still updates state and publishes the refresh event.
131-146:⚠️ Potential issue | 🟡 MinorEvent published regardless of rename success.
PluginRefreshEventis published at line 144 even when the file rename fails (line 141-143). If the rename fails, the plugin state hasn't changed, so triggering a UI refresh may be unnecessary. Consider moving the event publication inside the success branch, or verify this is the intended behavior.💡 Suggested fix if event should only fire on success
if (originFile.renameTo(destFile)) { plugin.setDisabled(true); plugin.setPath(plugin.getPath() + ".disabled"); pluginRepository.save(plugin); - + publisher.publishEvent(new PluginRefreshEvent()); } else { log.error("Plugin can't be disabled: failed to rename file {}", plugin.getPath()); } - publisher.publishEvent(new PluginRefreshEvent());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@owlplug-client/src/main/java/com/owlplug/plugin/services/PluginService.java` around lines 131 - 146, The PluginRefreshEvent is being published unconditionally in disablePlugin even if originFile.renameTo(destFile) fails; change the logic so that publisher.publishEvent(new PluginRefreshEvent()) is moved inside the success branch (after plugin.setDisabled(true), plugin.setPath(...), and pluginRepository.save(plugin)) so the refresh event is only fired when the rename and state update succeed; if the intent was to always notify, add an explicit comment explaining that behavior instead.
🧹 Nitpick comments (4)
owlplug-client/src/main/java/com/owlplug/auth/events/AccountUpdateEvent.java (1)
21-22: Consider adding Javadoc for consistency.Unlike
PluginUpdateEventwhich has a descriptive Javadoc, this record lacks documentation. Adding a brief Javadoc would improve consistency across event types.📝 Suggested Javadoc
+/** + * Account data has been updated (created, modified, or deleted). + */ public record AccountUpdateEvent() { }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@owlplug-client/src/main/java/com/owlplug/auth/events/AccountUpdateEvent.java` around lines 21 - 22, Add a brief Javadoc to the AccountUpdateEvent record to match the style and consistency of PluginUpdateEvent; open the AccountUpdateEvent record declaration (AccountUpdateEvent) and insert a short descriptive Javadoc comment above it describing its purpose (e.g., fired when an account is created, updated, or deleted) and any relevant usage notes or authored/see tags to mirror PluginUpdateEvent's documentation style.owlplug-client/src/main/java/com/owlplug/auth/services/AuthenticationService.java (1)
42-42: Remove unused import forMainController.The
MainControllerimport on line 42 is not referenced anywhere in the class. Following the refactoring to event-driven architecture, the service now usesApplicationEventPublisher.publishEvent()instead of direct controller calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@owlplug-client/src/main/java/com/owlplug/auth/services/AuthenticationService.java` at line 42, AuthenticationService contains an unused import for MainController; remove the unused import statement "import com.owlplug.core.controllers.MainController;" from the AuthenticationService class so imports reflect the event-driven refactor (the class now uses ApplicationEventPublisher.publishEvent()). Ensure no other references to MainController remain in AuthenticationService and run a compile to confirm imports are clean.owlplug-client/src/main/java/com/owlplug/core/utils/FX.java (1)
22-22: Unused import:PauseTransitionThis import is not used anywhere in the class.
🧹 Suggested fix
import java.util.concurrent.CompletableFuture; -import javafx.animation.PauseTransition; import javafx.application.Platform; -import javafx.util.Duration;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@owlplug-client/src/main/java/com/owlplug/core/utils/FX.java` at line 22, Remove the unused import of PauseTransition from FX.java; delete the line importing javafx.animation.PauseTransition so only used imports remain (e.g., keep imports used by methods/fields in class FX), ensuring no references to PauseTransition exist in the class before removing.owlplug-client/src/main/java/com/owlplug/core/components/DialogManager.java (1)
40-43: Consider adding a null check fordialogContainer.If
newDialog()is called beforesetDialogContainer()has been invoked (e.g., during early Spring bean initialization), thedialogContainerwill be null. Depending on how theDialogclass handles this, it could cause issues.🛡️ Proposed defensive check
public Dialog newDialog() { + if (dialogContainer == null) { + throw new IllegalStateException("Dialog container not initialized. Call setDialogContainer() first."); + } Dialog dialog = new Dialog(); dialog.setDialogContainer(dialogContainer); return dialog; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@owlplug-client/src/main/java/com/owlplug/core/components/DialogManager.java` around lines 40 - 43, newDialog() may attempt to set a null dialogContainer if setDialogContainer(...) hasn't been called; add a defensive null check in newDialog() that verifies dialogContainer != null and, if it is null, throw an IllegalStateException (or another appropriate runtime exception) with a clear message like "dialogContainer not initialized - call setDialogContainer(...) first" so callers fail fast; update references in newDialog() (and document behavior) rather than relying on Dialog to handle a null container.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@owlplug-client/src/main/java/com/owlplug/plugin/services/PluginService.java`:
- Around line 148-167: The PluginRefreshEvent is published regardless of whether
the rename succeeded in enablePlugin; move the publisher.publishEvent(new
PluginRefreshEvent()) so it only runs after a successful rename (i.e., inside
the if (originFile.renameTo(destFile)) block where plugin.setDisabled(false),
plugin.setPath(newPath), and pluginRepository.save(plugin) are executed); keep
the error log in the else branch unchanged so no event is emitted on failure and
the success branch still updates state and publishes the refresh event.
- Around line 131-146: The PluginRefreshEvent is being published unconditionally
in disablePlugin even if originFile.renameTo(destFile) fails; change the logic
so that publisher.publishEvent(new PluginRefreshEvent()) is moved inside the
success branch (after plugin.setDisabled(true), plugin.setPath(...), and
pluginRepository.save(plugin)) so the refresh event is only fired when the
rename and state update succeed; if the intent was to always notify, add an
explicit comment explaining that behavior instead.
---
Nitpick comments:
In
`@owlplug-client/src/main/java/com/owlplug/auth/events/AccountUpdateEvent.java`:
- Around line 21-22: Add a brief Javadoc to the AccountUpdateEvent record to
match the style and consistency of PluginUpdateEvent; open the
AccountUpdateEvent record declaration (AccountUpdateEvent) and insert a short
descriptive Javadoc comment above it describing its purpose (e.g., fired when an
account is created, updated, or deleted) and any relevant usage notes or
authored/see tags to mirror PluginUpdateEvent's documentation style.
In
`@owlplug-client/src/main/java/com/owlplug/auth/services/AuthenticationService.java`:
- Line 42: AuthenticationService contains an unused import for MainController;
remove the unused import statement "import
com.owlplug.core.controllers.MainController;" from the AuthenticationService
class so imports reflect the event-driven refactor (the class now uses
ApplicationEventPublisher.publishEvent()). Ensure no other references to
MainController remain in AuthenticationService and run a compile to confirm
imports are clean.
In `@owlplug-client/src/main/java/com/owlplug/core/components/DialogManager.java`:
- Around line 40-43: newDialog() may attempt to set a null dialogContainer if
setDialogContainer(...) hasn't been called; add a defensive null check in
newDialog() that verifies dialogContainer != null and, if it is null, throw an
IllegalStateException (or another appropriate runtime exception) with a clear
message like "dialogContainer not initialized - call setDialogContainer(...)
first" so callers fail fast; update references in newDialog() (and document
behavior) rather than relying on Dialog to handle a null container.
In `@owlplug-client/src/main/java/com/owlplug/core/utils/FX.java`:
- Line 22: Remove the unused import of PauseTransition from FX.java; delete the
line importing javafx.animation.PauseTransition so only used imports remain
(e.g., keep imports used by methods/fields in class FX), ensuring no references
to PauseTransition exist in the class before removing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 653c6e4c-e25a-4f63-8b89-abebd598d638
📒 Files selected for processing (50)
owlplug-client/src/main/java/com/owlplug/OwlPlug.javaowlplug-client/src/main/java/com/owlplug/auth/controllers/AccountController.javaowlplug-client/src/main/java/com/owlplug/auth/events/AccountUpdateEvent.javaowlplug-client/src/main/java/com/owlplug/auth/services/AuthenticationService.javaowlplug-client/src/main/java/com/owlplug/core/components/DialogManager.javaowlplug-client/src/main/java/com/owlplug/core/components/TaskRunner.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/CrashRecoveryDialogController.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/events/PreferencesChangedEvent.javaowlplug-client/src/main/java/com/owlplug/core/services/AppUpdateService.javaowlplug-client/src/main/java/com/owlplug/core/services/BaseService.javaowlplug-client/src/main/java/com/owlplug/core/services/OptionsService.javaowlplug-client/src/main/java/com/owlplug/core/services/TelemetryService.javaowlplug-client/src/main/java/com/owlplug/core/utils/FX.javaowlplug-client/src/main/java/com/owlplug/explore/controllers/ExploreController.javaowlplug-client/src/main/java/com/owlplug/explore/controllers/NewSourceDialogController.javaowlplug-client/src/main/java/com/owlplug/explore/controllers/PackageInfoController.javaowlplug-client/src/main/java/com/owlplug/explore/controllers/SourceMenuController.javaowlplug-client/src/main/java/com/owlplug/explore/controllers/dialogs/InstallStepDialogController.javaowlplug-client/src/main/java/com/owlplug/explore/events/RemoteSourceUpdatedEvent.javaowlplug-client/src/main/java/com/owlplug/explore/services/ExploreService.javaowlplug-client/src/main/java/com/owlplug/plugin/components/PluginTaskFactory.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/controllers/dialogs/DisablePluginDialogController.javaowlplug-client/src/main/java/com/owlplug/plugin/events/PluginRefreshEvent.javaowlplug-client/src/main/java/com/owlplug/plugin/events/PluginUpdateEvent.javaowlplug-client/src/main/java/com/owlplug/plugin/services/NativeHostService.javaowlplug-client/src/main/java/com/owlplug/plugin/services/PluginService.javaowlplug-client/src/main/java/com/owlplug/project/controllers/ProjectInfoController.javaowlplug-client/src/main/java/com/owlplug/project/controllers/ProjectsController.javaowlplug-client/src/main/java/com/owlplug/project/services/PluginLookupService.javaowlplug-client/src/main/resources/application.propertiesowlplug-client/src/main/resources/fxml/MainView.fxmlowlplug-client/src/main/resources/fxml/dialogs/ListDirectoryView.fxmlowlplug-client/src/main/resources/fxml/explore/ExploreView.fxmlowlplug-client/src/main/resources/fxml/explore/PackageInfoView.fxmlowlplug-client/src/main/resources/fxml/plugins/ComponentInfoView.fxmlowlplug-client/src/main/resources/fxml/plugins/DirectoryInfoView.fxmlowlplug-client/src/main/resources/fxml/plugins/NodeInfoView.fxmlowlplug-client/src/main/resources/fxml/plugins/PluginInfoView.fxmlowlplug-client/src/main/resources/fxml/plugins/PluginsView.fxmlowlplug-client/src/main/resources/fxml/plugins/SymlinkInfoView.fxmlowlplug-client/src/main/resources/fxml/projects/ProjectInfoView.fxmlowlplug-client/src/main/resources/fxml/projects/ProjectsView.fxml
💤 Files with no reviewable changes (6)
- owlplug-client/src/main/java/com/owlplug/explore/controllers/NewSourceDialogController.java
- owlplug-client/src/main/java/com/owlplug/auth/controllers/AccountController.java
- owlplug-client/src/main/java/com/owlplug/plugin/services/NativeHostService.java
- owlplug-client/src/main/java/com/owlplug/plugin/controllers/dialogs/DisablePluginDialogController.java
- owlplug-client/src/main/java/com/owlplug/core/services/AppUpdateService.java
- owlplug-client/src/main/java/com/owlplug/core/services/TelemetryService.java
🚧 Files skipped from review as they are similar to previous changes (20)
- owlplug-client/src/main/java/com/owlplug/plugin/components/PluginTaskFactory.java
- owlplug-client/src/main/java/com/owlplug/core/services/OptionsService.java
- owlplug-client/src/main/resources/fxml/plugins/DirectoryInfoView.fxml
- owlplug-client/src/main/resources/fxml/projects/ProjectsView.fxml
- owlplug-client/src/main/java/com/owlplug/explore/events/RemoteSourceUpdatedEvent.java
- owlplug-client/src/main/java/com/owlplug/core/controllers/OptionsController.java
- owlplug-client/src/main/java/com/owlplug/core/events/PreferencesChangedEvent.java
- owlplug-client/src/main/resources/fxml/plugins/PluginsView.fxml
- owlplug-client/src/main/resources/application.properties
- owlplug-client/src/main/java/com/owlplug/project/services/PluginLookupService.java
- owlplug-client/src/main/java/com/owlplug/explore/controllers/SourceMenuController.java
- owlplug-client/src/main/java/com/owlplug/core/components/TaskRunner.java
- owlplug-client/src/main/java/com/owlplug/plugin/events/PluginRefreshEvent.java
- owlplug-client/src/main/resources/fxml/plugins/ComponentInfoView.fxml
- owlplug-client/src/main/java/com/owlplug/OwlPlug.java
- owlplug-client/src/main/java/com/owlplug/core/services/BaseService.java
- owlplug-client/src/main/java/com/owlplug/explore/services/ExploreService.java
- owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginInfoController.java
- owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginTableController.java
- owlplug-client/src/main/resources/fxml/dialogs/ListDirectoryView.fxml
DropSnorz
force-pushed
the
chore/refactor-controller-di
branch
from
c83b1bf to
be35e46
Compare
DropSnorz
force-pushed
the
chore/refactor-controller-di
branch
from
be35e46 to
a02a95b
Compare
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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/explore/services/ExploreService.java (1)
256-258:⚠️ Potential issue | 🟠 MajorPublish event after
delete()for consistency withsave()andenableSource().The
delete()method does not publishRemoteSourceUpdatedEvent, unlikesave()(line 250-254) andenableSource()(line 244-248). Controllers includingSourceMenuController(line 155-157) andExploreController(line 405-407) have event listeners registered for this event. While the current caller manually invokesrefreshView()as a workaround, this inconsistency breaks the established service pattern and risks stale UI state if additional callers are added without manual refresh logic.♻️ Proposed fix
public void delete(RemoteSource remoteSource) { remoteSourceRepository.delete(remoteSource); + publisher.publishEvent(new RemoteSourceUpdatedEvent(remoteSource)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@owlplug-client/src/main/java/com/owlplug/explore/services/ExploreService.java` around lines 256 - 258, The delete(RemoteSource) method currently only calls remoteSourceRepository.delete(remoteSource) — update it to publish the same RemoteSourceUpdatedEvent as save() and enableSource() so listeners (e.g., SourceMenuController, ExploreController) receive updates; after performing the delete call, invoke the existing applicationEventPublisher.publishEvent(...) with a new RemoteSourceUpdatedEvent carrying the deleted RemoteSource (matching the pattern used in save() and enableSource()) so UI refreshes remain consistent.
🧹 Nitpick comments (1)
owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginInfoController.java (1)
265-269: Minor: Missing blank line before@EventListener.For consistency with other event listener declarations in the codebase, consider adding a blank line before the
@EventListenerannotation.Suggested formatting
} + `@EventListener` private void handle(PluginRefreshEvent event) { FX.run(this::refresh); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginInfoController.java` around lines 265 - 269, Add a blank line before the `@EventListener` annotation in PluginInfoController to match project formatting conventions; locate the private void handle(PluginRefreshEvent event) method (the event listener that calls FX.run(this::refresh)) and insert a single empty line immediately above the `@EventListener` annotation so it is separated from the previous method or block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@owlplug-client/src/main/java/com/owlplug/OwlPlug.java`:
- Line 170: Replace the byte-based heap sizing in the
ResourcePoolsBuilder.newResourcePoolsBuilder().heap(200, MemoryUnit.MB) call
with entry-based sizing using EntryUnit.ENTRIES and pick a sensible entries
limit; add a short inline comment near the ResourcePoolsBuilder invocation (in
OwlPlug.java) documenting the rationale for the chosen entry count (expected
average cache object size, concurrency, and eviction behavior) so future
reviewers understand why that number was chosen; ensure you import
org.ehcache.config.units.EntryUnit and remove or stop using MemoryUnit for heap
sizing to avoid the deprecated byte-based traversal.
---
Outside diff comments:
In
`@owlplug-client/src/main/java/com/owlplug/explore/services/ExploreService.java`:
- Around line 256-258: The delete(RemoteSource) method currently only calls
remoteSourceRepository.delete(remoteSource) — update it to publish the same
RemoteSourceUpdatedEvent as save() and enableSource() so listeners (e.g.,
SourceMenuController, ExploreController) receive updates; after performing the
delete call, invoke the existing applicationEventPublisher.publishEvent(...)
with a new RemoteSourceUpdatedEvent carrying the deleted RemoteSource (matching
the pattern used in save() and enableSource()) so UI refreshes remain
consistent.
---
Nitpick comments:
In
`@owlplug-client/src/main/java/com/owlplug/plugin/controllers/PluginInfoController.java`:
- Around line 265-269: Add a blank line before the `@EventListener` annotation in
PluginInfoController to match project formatting conventions; locate the private
void handle(PluginRefreshEvent event) method (the event listener that calls
FX.run(this::refresh)) and insert a single empty line immediately above the
`@EventListener` annotation so it is separated from the previous method or block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9dffa416-fedc-4624-b6f4-f92036501904
📒 Files selected for processing (50)
owlplug-client/src/main/java/com/owlplug/OwlPlug.javaowlplug-client/src/main/java/com/owlplug/auth/controllers/AccountController.javaowlplug-client/src/main/java/com/owlplug/auth/events/AccountUpdateEvent.javaowlplug-client/src/main/java/com/owlplug/auth/services/AuthenticationService.javaowlplug-client/src/main/java/com/owlplug/core/components/DialogManager.javaowlplug-client/src/main/java/com/owlplug/core/components/TaskRunner.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/CrashRecoveryDialogController.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/events/PreferencesChangedEvent.javaowlplug-client/src/main/java/com/owlplug/core/services/AppUpdateService.javaowlplug-client/src/main/java/com/owlplug/core/services/BaseService.javaowlplug-client/src/main/java/com/owlplug/core/services/OptionsService.javaowlplug-client/src/main/java/com/owlplug/core/services/TelemetryService.javaowlplug-client/src/main/java/com/owlplug/core/utils/FX.javaowlplug-client/src/main/java/com/owlplug/explore/controllers/ExploreController.javaowlplug-client/src/main/java/com/owlplug/explore/controllers/NewSourceDialogController.javaowlplug-client/src/main/java/com/owlplug/explore/controllers/PackageInfoController.javaowlplug-client/src/main/java/com/owlplug/explore/controllers/SourceMenuController.javaowlplug-client/src/main/java/com/owlplug/explore/controllers/dialogs/InstallStepDialogController.javaowlplug-client/src/main/java/com/owlplug/explore/events/RemoteSourceUpdatedEvent.javaowlplug-client/src/main/java/com/owlplug/explore/services/ExploreService.javaowlplug-client/src/main/java/com/owlplug/plugin/components/PluginTaskFactory.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/controllers/dialogs/DisablePluginDialogController.javaowlplug-client/src/main/java/com/owlplug/plugin/events/PluginRefreshEvent.javaowlplug-client/src/main/java/com/owlplug/plugin/events/PluginUpdateEvent.javaowlplug-client/src/main/java/com/owlplug/plugin/services/NativeHostService.javaowlplug-client/src/main/java/com/owlplug/plugin/services/PluginService.javaowlplug-client/src/main/java/com/owlplug/project/controllers/ProjectInfoController.javaowlplug-client/src/main/java/com/owlplug/project/controllers/ProjectsController.javaowlplug-client/src/main/java/com/owlplug/project/services/PluginLookupService.javaowlplug-client/src/main/resources/application.propertiesowlplug-client/src/main/resources/fxml/MainView.fxmlowlplug-client/src/main/resources/fxml/dialogs/ListDirectoryView.fxmlowlplug-client/src/main/resources/fxml/explore/ExploreView.fxmlowlplug-client/src/main/resources/fxml/explore/PackageInfoView.fxmlowlplug-client/src/main/resources/fxml/plugins/ComponentInfoView.fxmlowlplug-client/src/main/resources/fxml/plugins/DirectoryInfoView.fxmlowlplug-client/src/main/resources/fxml/plugins/NodeInfoView.fxmlowlplug-client/src/main/resources/fxml/plugins/PluginInfoView.fxmlowlplug-client/src/main/resources/fxml/plugins/PluginsView.fxmlowlplug-client/src/main/resources/fxml/plugins/SymlinkInfoView.fxmlowlplug-client/src/main/resources/fxml/projects/ProjectInfoView.fxmlowlplug-client/src/main/resources/fxml/projects/ProjectsView.fxml
💤 Files with no reviewable changes (6)
- owlplug-client/src/main/java/com/owlplug/core/services/AppUpdateService.java
- owlplug-client/src/main/java/com/owlplug/plugin/services/NativeHostService.java
- owlplug-client/src/main/java/com/owlplug/auth/controllers/AccountController.java
- owlplug-client/src/main/java/com/owlplug/plugin/controllers/dialogs/DisablePluginDialogController.java
- owlplug-client/src/main/java/com/owlplug/explore/controllers/NewSourceDialogController.java
- owlplug-client/src/main/java/com/owlplug/core/services/TelemetryService.java
✅ Files skipped from review due to trivial changes (21)
- owlplug-client/src/main/java/com/owlplug/core/services/BaseService.java
- owlplug-client/src/main/java/com/owlplug/core/services/OptionsService.java
- owlplug-client/src/main/java/com/owlplug/core/events/PreferencesChangedEvent.java
- owlplug-client/src/main/java/com/owlplug/core/controllers/fragments/PluginPathFragmentController.java
- owlplug-client/src/main/java/com/owlplug/explore/controllers/dialogs/InstallStepDialogController.java
- owlplug-client/src/main/resources/application.properties
- owlplug-client/src/main/java/com/owlplug/auth/events/AccountUpdateEvent.java
- owlplug-client/src/main/resources/fxml/plugins/PluginInfoView.fxml
- owlplug-client/src/main/resources/fxml/plugins/ComponentInfoView.fxml
- owlplug-client/src/main/resources/fxml/projects/ProjectsView.fxml
- owlplug-client/src/main/resources/fxml/plugins/SymlinkInfoView.fxml
- owlplug-client/src/main/resources/fxml/plugins/PluginsView.fxml
- owlplug-client/src/main/java/com/owlplug/project/controllers/ProjectInfoController.java
- owlplug-client/src/main/resources/fxml/MainView.fxml
- owlplug-client/src/main/resources/fxml/projects/ProjectInfoView.fxml
- owlplug-client/src/main/resources/fxml/explore/ExploreView.fxml
- owlplug-client/src/main/java/com/owlplug/plugin/events/PluginRefreshEvent.java
- owlplug-client/src/main/java/com/owlplug/explore/events/RemoteSourceUpdatedEvent.java
- owlplug-client/src/main/resources/fxml/dialogs/ListDirectoryView.fxml
- owlplug-client/src/main/java/com/owlplug/core/utils/FX.java
- owlplug-client/src/main/java/com/owlplug/project/controllers/ProjectsController.java
🚧 Files skipped from review as they are similar to previous changes (8)
- owlplug-client/src/main/resources/fxml/plugins/DirectoryInfoView.fxml
- owlplug-client/src/main/java/com/owlplug/core/components/TaskRunner.java
- owlplug-client/src/main/java/com/owlplug/plugin/components/PluginTaskFactory.java
- owlplug-client/src/main/java/com/owlplug/project/services/PluginLookupService.java
- owlplug-client/src/main/java/com/owlplug/auth/services/AuthenticationService.java
- owlplug-client/src/main/java/com/owlplug/explore/controllers/SourceMenuController.java
- owlplug-client/src/main/java/com/owlplug/plugin/events/PluginUpdateEvent.java
- owlplug-client/src/main/java/com/owlplug/core/controllers/dialogs/CrashRecoveryDialogController.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.
Relates to #437