feat(plugins): dashboard UI for plugin management - [deferred to v4.0.0-rc1] #2386
feat(plugins): dashboard UI for plugin management - [deferred to v4.0.0-rc1] #2386oyi77 wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a plugin system for OmniRoute, featuring lifecycle management, sandboxed execution via Node.js VM, and a dashboard UI. The review highlights critical issues in the plugin loader regarding ESM compatibility and unawaited promises, as well as performance overhead from dynamic imports in the chat core. Additionally, the changes violate repository standards regarding test coverage and centralized Zod schema placement. Feedback also suggests improving API request parsing and the plugin scanner's synchronization logic.
| // Only allow specific safe modules | ||
| const allowed: Record<string, unknown> = {}; | ||
| if (id === "crypto") { | ||
| allowed.crypto = require("crypto"); |
There was a problem hiding this comment.
Using require("crypto") will fail if the host environment is running in ESM mode (which is the case for this project). You should use the imported crypto module instead.
| allowed.crypto = require("crypto"); | |
| allowed.crypto = crypto; |
| vm.runInContext(wrapped, context, { | ||
| filename: entryPoint, | ||
| timeout: 10000, // 10s init timeout | ||
| }); |
There was a problem hiding this comment.
The wrapped source is an async function IIFE, which returns a Promise. vm.runInContext returns this promise, but it is not awaited. Consequently, the code following this block will execute before the plugin has finished initializing its exports, which will likely result in hooks being undefined when checked on line 134.
| vm.runInContext(wrapped, context, { | |
| filename: entryPoint, | |
| timeout: 10000, // 10s init timeout | |
| }); | |
| await vm.runInContext(wrapped, context, { | |
| filename: entryPoint, | |
| timeout: 10000, // 10s init timeout | |
| }); |
|
|
||
| // ── Plugin onRequest hook ── | ||
| try { | ||
| const { runOnRequest } = await import("@/lib/plugins/index"); |
There was a problem hiding this comment.
Dynamic imports inside the main request handler (handleChatCore) introduce unnecessary overhead on every request. Consider moving this to a top-level import or implementing a lazy-loading pattern that caches the module after the first load to improve performance in this hot path.
| } catch (error) { | ||
| // ── Plugin onError hook ── | ||
| try { | ||
| const { runOnError } = await import("@/lib/plugins/index"); |
There was a problem hiding this comment.
Dynamic import of the plugin module inside the error handler. Similar to the onRequest hook, this should be optimized to avoid per-request overhead.
| */ | ||
|
|
||
| import { readFile } from "fs/promises"; | ||
| import * as vm from "vm"; |
There was a problem hiding this comment.
Import the crypto module here to avoid using require inside the sandbox helper, which is incompatible with ESM environments.
| import * as vm from "vm"; | |
| import * as vm from "vm"; | |
| import * as crypto from "crypto"; |
| * Install a plugin from a source directory. | ||
| * Copies to plugin dir, validates manifest, registers in DB. | ||
| */ | ||
| async install(sourceDir: string): Promise<PluginRow> { |
There was a problem hiding this comment.
This PR adds significant new production logic for plugin management but lacks any associated tests. Per repository rule 2.9, all changes to production code must include tests to maintain quality and coverage.
References
- Always include tests when changing production code (src/, open-sse/, electron/, bin/). (link)
| * Scan plugin directory and sync with DB. | ||
| * Discovers new plugins and marks missing ones. | ||
| */ | ||
| async scan(): Promise<{ discovered: number; errors: Array<{ name: string; error: string }> }> { |
There was a problem hiding this comment.
The scan() method only discovers new plugins and adds them to the database. It does not handle syncing existing plugins (e.g., updating metadata if the manifest changed) or marking plugins as missing/error if their directory was deleted from the filesystem. This could lead to stale data in the dashboard.
| // ── Full manifest ── | ||
|
|
||
| export const PluginManifestSchema = z.object({ | ||
| name: z |
There was a problem hiding this comment.
Plugin manifest schemas are defined locally in this file. According to repository rule 2.8, all Zod schemas used for input validation should be centralized in src/shared/validation/schemas.ts.
References
- Always validate inputs with Zod schemas from src/shared/validation/schemas.ts. (link)
| */ | ||
| export async function PUT(request: NextRequest, { params }: { params: Promise<{ name: string }> }) { | ||
| const { name } = await params; | ||
| const body = await request.json(); |
There was a problem hiding this comment.
Directly awaiting request.json() without error handling will cause a 500 error if the request body is empty or invalid JSON. It should be wrapped in a try-catch block to return a 400 Bad Request instead.
| const body = await request.json(); | |
| let body; | |
| try { | |
| body = await request.json(); | |
| } catch (e) { | |
| return NextResponse.json({ error: "Invalid JSON body" }, { status: 400, headers: CORS_HEADERS }); | |
| } |
diegosouzapw
changed the title
2 participants
Summary
Adds plugin management dashboard UI (Wave 4 of #2060).
Depends on: #2385 (plugin backend core)
What's included:
/dashboard/plugins— plugin list with install, activate/deactivate, uninstall, scan/dashboard/plugins/[name]/config— auto-generated config form from plugin's configSchemaTest plan
/dashboard/plugins