feat(plugins): Add UI feedback for plugin operations and refactor han…#51
Merged
feat(plugins): Add UI feedback for plugin operations and refactor han…#51
Conversation
…dler
Implemented toast notifications in the UI for plugin installation and uninstallation, providing immediate user feedback with success or error messages. This improves the user experience by making plugin operations more transparent.
Refactored the plugin handler's `savePlugin` method to enhance readability, maintainability, and error handling. Key logic has been extracted into dedicated private helper functions: `isDuplicatePlugin`, `ensurePluginBundle`, `checkPluginSafety`, `updateExistingPlugin`, and `insertNewPlugin`. All plugin operation results now return a consistent `{ success: boolean, message: string }` object.
Additionally, the `verificationApi` column was removed from the plugin database schema, and the `Toast` component's default button variant was updated from "outline" to "secondary" for non-error messages.
Contributor
Reviewer's GuideAdds user-facing toast notifications for plugin install/uninstall operations, refactors plugin saving logic into smaller helpers with consistent result objects, removes an unused DB column, and adjusts the default toast button style for non-error messages. Sequence diagram for plugin install with UI toast feedbacksequenceDiagram
actor User
participant PluginBrowser
participant ReactQuery
participant Backend as PluginHandler
participant Toast
User->>PluginBrowser: Click install on plugin
PluginBrowser->>ReactQuery: mutateAsync(pluginPayload)
ReactQuery->>Backend: savePlugin(plugin, update=false)
Backend->>Backend: isDuplicatePlugin(name)
alt duplicate plugin
Backend-->>ReactQuery: { success: false, message }
else not duplicate
Backend->>Backend: ensurePluginBundle(plugin)
alt bundle fetch fails
Backend-->>ReactQuery: { success: false, message }
else bundle ok
Backend->>Backend: checkPluginSafety(plugin)
alt plugin unsafe or unverified
Backend-->>ReactQuery: { success: false, message }
else plugin safe
Backend->>Backend: insertNewPlugin(plugin)
Backend-->>ReactQuery: { success: true, message, id }
end
end
end
ReactQuery-->>PluginBrowser: mutation result
PluginBrowser->>Toast: toast({ title, description: result.message, variant })
Toast-->>User: Visual notification of install success or error
Sequence diagram for plugin uninstall with UI toast feedbacksequenceDiagram
actor User
participant PluginBrowser
participant deletePluginMutation as ReactQuery
participant PluginHandler as Backend
participant toast as Toast
User->>PluginBrowser: Click uninstall on plugin
PluginBrowser->>deletePluginMutation: mutateAsync(id)
deletePluginMutation->>PluginHandler: deletePlugin(id)
PluginHandler-->>deletePluginMutation: { success: boolean, message: string }
deletePluginMutation-->>PluginBrowser: mutation result
PluginBrowser->>toast: toast({ title, description: result.message, variant })
toast-->>User: Visual notification of uninstall success or error
ER diagram for updated plugin table without verificationApierDiagram
PLUGIN {
int id PK
text name
text repoType
text repository
text manifest
json author
text plugin
}
Class diagram for refactored PluginHandler savePlugin workflowclassDiagram
class PluginHandler {
- logger
- table
+ savePlugin(plugin: DBPluginShemaT, update: boolean) Result
- isDuplicatePlugin(name: string) boolean
- ensurePluginBundle(plugin: DBPluginShemaT) Result
- checkPluginSafety(plugin: DBPluginShemaT) Result
- updateExistingPlugin(plugin: DBPluginShemaT) Result
- insertNewPlugin(plugin: DBPluginShemaT) Result
+ getAll() DBPluginShemaT[]
+ verifyPlugin(plugin: DBPluginShemaT) VerificationResult
+ unloadPlugin(id: number) void
+ deletePlugin(id: number) void
+ loadPlugin(id: number) void
}
class DBPluginShemaT {
+ id: number
+ name: string
+ repoType: string
+ repository: string
+ manifest: string
+ author: json
+ plugin: string
}
class Result {
+ success: boolean
+ message: string
+ id: number
}
class VerificationResult {
<<union>>
+ value: number
+ value: boolean
+ value: string
}
PluginHandler --> DBPluginShemaT : manages
PluginHandler --> Result : returns
PluginHandler --> VerificationResult : uses
File-Level Changes
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 4 issues, and left some high level feedback:
- The helper methods used by
savePlugin(ensurePluginBundle,checkPluginSafety,insertNewPlugin,updateExistingPlugin) don’t consistently return the{ success: boolean, message: string }shape described in the PR (e.g., some success paths omitmessage), which makes the API harder to reason about and could break callers expecting a uniform result object. updateExistingPluginassumesinsertNewPluginreturns an object with anidfield (res.id && this.loadPlugin(res.id)), butinsertNewPluginappears to still return only{ success, message }and already performsloadPlugin, so this code path is either redundant or incorrect and should be aligned with the actual return value.- The install/uninstall toasts always use a success-oriented title (
Installed X,Uninstalled PluginID: Y) even on failure; consider adjusting the title based onres.successso users get clearer feedback when an operation fails.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The helper methods used by `savePlugin` (`ensurePluginBundle`, `checkPluginSafety`, `insertNewPlugin`, `updateExistingPlugin`) don’t consistently return the `{ success: boolean, message: string }` shape described in the PR (e.g., some success paths omit `message`), which makes the API harder to reason about and could break callers expecting a uniform result object.
- `updateExistingPlugin` assumes `insertNewPlugin` returns an object with an `id` field (`res.id && this.loadPlugin(res.id)`), but `insertNewPlugin` appears to still return only `{ success, message }` and already performs `loadPlugin`, so this code path is either redundant or incorrect and should be aligned with the actual return value.
- The install/uninstall toasts always use a success-oriented title (`Installed X`, `Uninstalled PluginID: Y`) even on failure; consider adjusting the title based on `res.success` so users get clearer feedback when an operation fails.
## Individual Comments
### Comment 1
<location> `packages/plugin-handler/src/index.ts:194-203` </location>
<code_context>
+ return { success: true }
+ }
+
+ try {
+ this.logger.info(`Plugin ${plugin.name} has no bundle, fetching...`)
+ plugin.plugin = await retry(
+ () => repo.getPluginBundle(plugin.repoType, `${plugin.repository}/${plugin.manifest}`),
{ attempts: 3, delay: 2000 }
)
+ return { success: true }
+ } catch (error) {
+ return {
+ success: false,
+ message: `Failed to fetch plugin bundle: ${JSON.stringify(error)}`,
+ }
}
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Serializing the error with `JSON.stringify` can be brittle and may expose internal details.
`JSON.stringify(error)` can throw on circular references and risks exposing sensitive internals (stack traces, config) in user-facing responses. Instead, log the full error and return a generic message:
```ts
} catch (error) {
this.logger.error({ error }, `Failed to fetch plugin bundle for ${plugin.name}`)
return {
success: false,
message: "Failed to fetch plugin bundle. Please try again later.",
}
}
```
If you need limited details in the response, use `String(error)` or `error instanceof Error ? error.message : String(error)` for the message field.
</issue_to_address>
### Comment 2
<location> `packages/plugin-handler/src/index.ts:235-241` </location>
<code_context>
+ return { success: false, message }
+ }
+
+ private updateExistingPlugin(plugin: DBPluginShemaT) {
+ this.logger.info(`Updating Plugin ${plugin.name}`)
+ this.unloadPlugin(Number(plugin.id))
+ this.deletePlugin(Number(plugin.id))
+ const res = this.insertNewPlugin(plugin)
+ res.id && this.loadPlugin(res.id)
+ return res
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Updating by delete-then-insert can leave the system without the plugin if insertion fails.
The current flow:
```ts
this.unloadPlugin(Number(plugin.id))
this.deletePlugin(Number(plugin.id))
const res = this.insertNewPlugin(plugin)
res.id && this.loadPlugin(res.id)
```
means that if `insertNewPlugin` fails, the plugin is permanently removed during an "update". To avoid this, consider:
1. Updating the existing row in place where possible, or
2. Inserting the new plugin first, then deleting the old one (with rollback/error handling), or
3. At least making the error clearly state that the previous version was removed and the new one failed.
If this behavior is intentional, also gate `loadPlugin` on an explicit success flag rather than just `res.id` to avoid loading an invalid/partial record.
</issue_to_address>
### Comment 3
<location> `apps/dockstat/src/pages/extensions/plugins.tsx:79-74` </location>
<code_context>
const handleInstall = async (plugin: AvailablePlugin) => {
- await installPluginMutation.mutateAsync({
+ const res = await installPluginMutation.mutateAsync({
id: plugin.isInstalled ? null : null,
plugin: "", // handled by backend
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The toast title always says "Installed" even when the installation fails.
Since `variant` can be `"error"`, consider making the title depend on `res.success`, for example:
```ts
toast({
title: res.success ? `Installed ${plugin.name}` : `Failed to install ${plugin.name}`,
description: res.message,
variant: res.success ? "success" : "error",
})
```
</issue_to_address>
### Comment 4
<location> `apps/dockstat/src/pages/extensions/plugins.tsx:80` </location>
<code_context>
const handleInstall = async (plugin: AvailablePlugin) => {
- await installPluginMutation.mutateAsync({
+ const res = await installPluginMutation.mutateAsync({
id: plugin.isInstalled ? null : null,
plugin: "", // handled by backend
name: plugin.name,
</code_context>
<issue_to_address>
**issue (bug_risk):** The ternary `plugin.isInstalled ? null : null` is redundant and can be simplified.
This always evaluates to `null`, so you can replace it with `id: null`. If different values are expected based on `plugin.isInstalled`, this is likely a typo and the branches should be updated accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Ensure correct `installedId` is passed when attempting to update/reinstall an existing plugin. - Add robust error handling during plugin updates to prevent inconsistent states if the previous version fails to uninstall. - Improve toast messages for plugin install/uninstall success and failure scenarios, providing clearer feedback to the user. - Format error messages more gracefully when fetching plugin bundles, displaying the error message directly instead of stringifying the entire error object.
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.
…dler
Implemented toast notifications in the UI for plugin installation and uninstallation, providing immediate user feedback with success or error messages. This improves the user experience by making plugin operations more transparent.
Refactored the plugin handler's
savePluginmethod to enhance readability, maintainability, and error handling. Key logic has been extracted into dedicated private helper functions:isDuplicatePlugin,ensurePluginBundle,checkPluginSafety,updateExistingPlugin, andinsertNewPlugin. All plugin operation results now return a consistent{ success: boolean, message: string }object.Additionally, the
verificationApicolumn was removed from the plugin database schema, and theToastcomponent's default button variant was updated from "outline" to "secondary" for non-error messages.Summary by Sourcery
Add user-facing feedback for plugin install/uninstall operations and refactor plugin persistence and safety checks for clearer flow and consistent result reporting.
New Features:
Enhancements: