Conversation
…nload and UI refresh This commit enhances the plugin management workflow by introducing automatic loading and unloading of plugins. - **Automatic Loading**: Newly installed plugins are now automatically loaded into memory via `loadPlugin` after insertion, ensuring they are immediately active without requiring a restart. - **Automatic Unloading**: Deleted plugins are now automatically unloaded from memory via `unloadPlugin`, ensuring resources are freed and their routes are removed from the system. - **Frontend UI Refresh**: On plugin installation or deletion, `fetchFrontendPluginRoutes` is invalidated, ensuring the UI immediately reflects changes in available plugin routes and extensions. - Removed a debug `console.log` from `getPluginBundle`.
Contributor
Reviewer's GuideImplements automatic plugin load/unload on create/delete in the backend and ensures the frontend query cache for plugin routes is invalidated alongside the plugin list, while removing a leftover debug log from the plugin bundle fetch utility. Sequence diagram for automatic plugin installation and loadingsequenceDiagram
actor User
participant PluginBrowser
participant QueryClient
participant Backend
participant PluginHandler
participant PluginRuntime
participant FrontendRouter
User->>PluginBrowser: Click install plugin
PluginBrowser->>Backend: installPlugin request
Backend->>PluginHandler: savePlugin(plugin)
alt plugin exists
PluginHandler->>PluginHandler: updateExistingPlugin
else new plugin
PluginHandler->>PluginHandler: insertNewPlugin
PluginHandler-->>PluginHandler: res(id, success, message)
opt res.id present
PluginHandler->>PluginHandler: loadPlugin(id)
PluginHandler->>PluginRuntime: register plugin and routes
PluginRuntime-->>PluginHandler: plugin loaded
end
PluginHandler-->>Backend: { id, success, message with load status }
end
Backend-->>PluginBrowser: installPlugin response
PluginBrowser->>QueryClient: onSuccess installPluginMutation
QueryClient->>QueryClient: invalidateQueries fetchAllPlugins
QueryClient->>QueryClient: invalidateQueries fetchFrontendPluginRoutes
QueryClient->>Backend: refetch fetchAllPlugins
Backend-->>QueryClient: updated plugin list
QueryClient->>Backend: refetch fetchFrontendPluginRoutes
Backend->>FrontendRouter: compute frontend plugin routes
Backend-->>QueryClient: updated plugin routes
QueryClient-->>PluginBrowser: updated data
PluginBrowser-->>User: UI shows new plugin and routes
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
savePlugin,insertNewPluginis now assigned toconst res = this.insertNewPlugin(plugin)withoutawait, so if it returns a promise you’ll be checkingres.idon the promise object instead of the resolved result; consider making thisconst res = await this.insertNewPlugin(plugin). - The
unloadPlugin(id)call in the delete path is not awaited or wrapped in a try/catch, which means any async errors will be unhandled and theunloadedflag may not represent the real outcome; consider making thisawaitwith error handling ifunloadPluginis asynchronous.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `savePlugin`, `insertNewPlugin` is now assigned to `const res = this.insertNewPlugin(plugin)` without `await`, so if it returns a promise you’ll be checking `res.id` on the promise object instead of the resolved result; consider making this `const res = await this.insertNewPlugin(plugin)`.
- The `unloadPlugin(id)` call in the delete path is not awaited or wrapped in a try/catch, which means any async errors will be unhandled and the `unloaded` flag may not represent the real outcome; consider making this `await` with error handling if `unloadPlugin` is asynchronous.
## Individual Comments
### Comment 1
<location> `packages/plugin-handler/src/index.ts:182-191` </location>
<code_context>
- return this.insertNewPlugin(plugin)
+ let loadedPlugin = false
+
+ const res = this.insertNewPlugin(plugin)
+ if (res.id) {
+ try {
+ await this.loadPlugin(res.id)
+ loadedPlugin = true
+ } catch (err) {
+ loadedPlugin = false
+ this.logger.error(
+ `Could not load plugin on save ${err instanceof Error ? err.message : String(err)}`
+ )
+ }
+ }
+
+ return { ...res, message: `${res.message} - loaded plugin: ${loadedPlugin}` }
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential missing `await` if `insertNewPlugin` is asynchronous.
This used to return `this.insertNewPlugin(plugin)` directly, implying `insertNewPlugin` likely returns a Promise. Now the result is stored in `res` and used synchronously (`res.id`, spread in the return) while `await` is only used on `loadPlugin`. If `insertNewPlugin` is async, `res` will be a Promise and this logic will fail. In that case, this should be `const res = await this.insertNewPlugin(plugin)` to match previous behavior and ensure `res.id` and the spread work correctly. If it is truly synchronous, please confirm, as that would be unusual for an insert operation.
</issue_to_address>
### Comment 2
<location> `packages/plugin-handler/src/index.ts:290` </location>
<code_context>
this.table.where({ id: id }).delete()
this.logger.info(`Deleted Plugin: ${id}`)
+
+ const unloaded = this.unloadPlugin(id)
+
return {
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider awaiting `unloadPlugin` if it is asynchronous to avoid unhandled errors.
If `unloadPlugin` is async and returns a Promise, calling it without `await` means any rejection won’t be caught by this `try/catch`, and the response may be sent before unload completes. If it’s async, use `const unloaded = await this.unloadPlugin(id)` so errors are properly handled; if it’s guaranteed sync, this is fine.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Its4Nik
deleted the
feat-automatic-loading-on-install-and-frontend-route-refresh
branch
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.
…nload and UI refresh
This commit enhances the plugin management workflow by introducing automatic loading and unloading of plugins.
loadPluginafter insertion, ensuring they are immediately active without requiring a restart.unloadPlugin, ensuring resources are freed and their routes are removed from the system.fetchFrontendPluginRoutesis invalidated, ensuring the UI immediately reflects changes in available plugin routes and extensions.console.logfromgetPluginBundle.Summary by Sourcery
Streamline plugin lifecycle by auto-loading on installation, unloading on deletion, and refreshing frontend plugin routes.
New Features:
Enhancements: