feat(cli): plugin management commands — add, list, remove - #606
Conversation
New CLI commands for managing external chart and connector plugins: neoboard plugin add <package> Install, validate, register neoboard plugin list Show built-in + external plugins neoboard plugin remove <package> Unregister and uninstall Validation pipeline (runs after npm install): - Checks package resolves and has a valid export - Validates required fields: type, label - Auto-detects chart (has transform) vs connector (has createModule) - Chart: validates compatibleWith array - Connector: validates category enum - Auto-rollback: uninstalls package on validation failure Infrastructure: - cli/src/lib/plugin-validator.ts — export validation (10 tests) - cli/src/lib/manifest.ts — read/write/remove manifest entries (9 tests) - cli/src/commands/plugin.ts — add, list, remove implementations Closes #605 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughThis PR introduces a complete plugin management system for the CLI, enabling users to install, validate, list, and remove external plugins (charts and connectors) through dedicated commands without manually editing JSON manifests. The system includes automatic type detection, comprehensive validation, and auto-rollback on failure. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant npm as npm Registry
participant Validator
participant Manifest
participant Codegen
User->>CLI: plugin add `@org/plugin` [--override] [--export name]
CLI->>npm: npm install `@org/plugin`
npm-->>CLI: ✓ Installed
CLI->>CLI: Dynamically import specified export
CLI->>Validator: validatePluginExport(exported)
Validator-->>CLI: { valid, errors, pluginType }
alt Validation Fails
CLI->>npm: npm uninstall `@org/plugin`
CLI-->>User: ❌ Validation errors + auto-rollback
else Validation Succeeds
CLI->>Manifest: addToManifest(filePath, key, entry)
Manifest-->>CLI: ✓ Added to manifest
CLI->>Codegen: Run generate-plugin-imports/generate-connector-imports
Codegen-->>CLI: ✓ Code generation complete
CLI-->>User: ✅ Plugin registered
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
cli/src/__tests__/lib/manifest.test.ts (2)
87-96: Missing coverage for theexportfield.
addToManifestacceptsexportperManifestEntry, andrunPluginAddwrites it when--exportdiffers from"default", but no test asserts it is persisted. Suggest adding an analogous"supports export field"case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/__tests__/lib/manifest.test.ts` around lines 87 - 96, Add a test mirroring "supports overrides flag" to cover the export field: call addToManifest(...) with an entry that includes export set to a non-default value (e.g., "named"), then call readManifest(...) and assert the returned entry's export equals that value; reference the existing helpers addToManifest and readManifest and the behavior in runPluginAdd/ManifestEntry so the test verifies the export field is persisted when it differs from "default".
14-17: Temp dir name uses onlyDate.now()— collision risk across parallel tests.Vitest may run tests in parallel; two
beforeEachhooks invoked within the same millisecond share a directory, which can flake under retries. Considercrypto.randomUUID()ormkdtempSyncfor a guaranteed-unique path.♻️ Proposed fix
-import { writeFileSync, mkdirSync, rmSync } from "node:fs"; +import { writeFileSync, mkdtempSync, rmSync } from "node:fs"; @@ beforeEach(() => { - tempDir = join(tmpdir(), "neoboard-test-" + Date.now()); - mkdirSync(tempDir, { recursive: true }); + tempDir = mkdtempSync(join(tmpdir(), "neoboard-test-")); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/__tests__/lib/manifest.test.ts` around lines 14 - 17, The temp directory name created in the beforeEach uses Date.now() which can collide in parallel tests; update the setup to produce a truly unique temp directory by replacing the join(tmpdir(), "neoboard-test-" + Date.now()) logic with a UUID or mkdtemp-based creation (e.g., use crypto.randomUUID() when constructing the directory name or call fs.mkdtempSync with a "neoboard-test-" prefix) and keep the mkdirSync(tempDir, { recursive: true }) or let mkdtempSync create the dir directly; edit the beforeEach that assigns tempDir and the code using join/tmpdir/mkdirSync to ensure uniqueness.cli/src/lib/plugin-validator.ts (1)
50-67: Re-derivinghasTransform/hasCreateModuleduplicatesdetectPluginType.After
detectPluginType(obj)returnsnull, lines 53-54 recompute the same predicates to decide between "ambiguous" and "missing". Consider havingdetectPluginTypereturn a tagged result (or export a helper), or just distinguish via a second helper — saves the duplicate logic and keeps the two sources of truth aligned if the heuristic ever evolves (e.g. adding new plugin shapes).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/lib/plugin-validator.ts` around lines 50 - 67, The code duplicates the plugin-shape checks after calling detectPluginType(obj); change detectPluginType (or add/export a small helper) so it returns a tagged result you can switch on (e.g. "chart" | "connector" | "ambiguous" | "none") or expose hasTransform/hasCreateModule helpers and use those directly instead of re-evaluating; then replace the current pluginType null branch in validate logic to use the tagged result (or the shared helpers) to decide between the ambiguous vs missing error messages, ensuring the shape-detection logic lives in one place (functions: detectPluginType, hasTransform, hasCreateModule).cli/src/__tests__/lib/plugin-validator.test.ts (1)
33-37: Missing test cases for array export and the ambiguous path invalidatePluginExport.The validator has explicit branches for
Array.isArray(exported)(line 34) and for thehasTransform && hasCreateModuleambiguity message (lines 55–59), but neither is exercised here. Both are easy to add and catch regressions in the specific user-facing messages.♻️ Proposed additions
+ it("rejects array export", () => { + const result = validatePluginExport([]); + expect(result.valid).toBe(false); + expect(result.errors[0]).toContain("must be an object"); + }); + + it("rejects ambiguous plugin with both transform and createModule", () => { + const result = validatePluginExport({ + type: "x", + label: "X", + transform: () => {}, + createModule: () => ({}), + }); + expect(result.valid).toBe(false); + expect(result.errors[0]).toContain("Ambiguous plugin"); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/__tests__/lib/plugin-validator.test.ts` around lines 33 - 37, Add two tests to cli/src/__tests__/lib/plugin-validator.test.ts that exercise the Array.isArray branch and the ambiguous hasTransform && hasCreateModule branch in validatePluginExport: one should call validatePluginExport([]) and assert result.valid === false and that result.errors includes the "must be an object" message (or the same message asserted for null), and the other should construct an export object containing both transform and createModule properties, call validatePluginExport(exported), assert result.valid === false and that result.errors contains the specific ambiguity message emitted by validatePluginExport (the message describing that both 'transform' and 'createModule' cannot be present).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/commands/plugin.ts`:
- Around line 98-128: The code continues to run codegen and print a success
message even when addToManifest(...) returns false (duplicate) and warn(...) is
called; update the flow in the command handler to stop further processing for
the duplicate case by returning early (or, if behavior should allow replacing
when an overrides flag is set, check the overrides variable/option before
proceeding) — specifically, after detecting added === false for
addToManifest(manifestPath, manifestKey, entry) call, either return immediately
(prevent run("node " + codegenScript, ...) and success(...)) or only continue
when an explicit overrides option is true so codegenScript and success('Plugin
"' + pluginLabel + '" registered as ' + pluginType + ...) are only executed for
actual changes.
- Around line 16-19: The import list in plugin.ts includes detectPluginType but
it is unused; remove detectPluginType from the named imports so only
validatePluginExport is imported (validation.pluginType already provides the
pluginType), e.g., update the import statement to drop detectPluginType to
satisfy ESLint and avoid dead code.
- Around line 36-46: The code concatenates user-controlled packageName into a
shell string passed to run(), causing shell injection; change calls like
run("npm install " + packageName, ...) to an argv-style invocation so arguments
are passed separately (e.g., run('npm', ['install', packageName], {cwd: root})
or call execFileSync('npm', ['install', packageName'], {cwd: root, shell:
false}) directly), and update run() in exec.ts to support an argv form that uses
execFileSync with shell: false; apply the same refactor for the npm uninstall
calls and any node <script> invocations that use codegenScript (pass the script
as a separate arg array rather than interpolating it into a shell command).
- Around line 134-157: Replace the hardcoded built-in lists in runPluginList:
remove the local builtInCharts and builtInConnectors arrays and import the
canonical CHART_TYPES and CONNECTOR_TYPES arrays from their source modules, then
use CHART_TYPES and CONNECTOR_TYPES in place of builtInCharts/builtInConnectors;
ensure you add the corresponding imports at the top of the file and update any
references inside runPluginList to use CHART_TYPES and CONNECTOR_TYPES (preserve
any formatting/filtering logic currently applied to the hardcoded arrays).
- Around line 48-58: The dynamic ESM import of packageName (await
import(packageName)) fails when the CLI runs outside the user's project; replace
it by creating a require bound to the project root using
createRequire(pathToFileURL(join(root, "package.json"))) and use that require to
load the package so resolution happens from root, then set exported the same way
(exportName === "default" ? (mod.default ?? mod) : mod[exportName]); keep the
existing try/catch and call rollback(packageName, root) on error as before.
In `@cli/src/lib/manifest.ts`:
- Around line 23-28: readManifest currently returns raw array items as
ManifestEntry[] which lets malformed entries (numbers/null/objects missing
package) reach callers like runPluginList and addToManifest; update readManifest
to validate and filter the parsed array so it only returns entries where typeof
e === "object" && e !== null && typeof (e as any).package === "string" (and any
other required fields), then map/cast to ManifestEntry[] before returning to
prevent undefined package accesses and bogus rows.
- Around line 35-47: addToManifest (and the analogous removeFromManifest)
currently overwrite the entire manifest with only { [key]: entries }, dropping
top-level keys like "$schema"; instead read and parse the existing JSON object
(using the existing readManifest helper to get entries but also JSON.parse the
file), replace only the target property (e.g., manifest[key] = entries or
updated entries), and write the full manifest object back with
JSON.stringify(...) to preserve other top-level keys (ensure writeFileSync is
still used and keep pretty-printing and trailing newline).
In `@cli/src/lib/plugin-validator.ts`:
- Around line 70-76: The chart plugin validator currently only checks
compatibleWith length and misses verifying a required React component; update
the plugin validation inside the pluginType === "chart" branch (the block that
references obj.compatibleWith) to also ensure obj.component is present and is a
callable/component type (e.g., a function or object consistent with
React.ComponentType) and to validate each entry of obj.compatibleWith is a
non-empty string (reject numbers/null/other types). Add clear error messages to
errors.push when component is missing/invalid and when any compatibleWith entry
is not a string or is empty.
---
Nitpick comments:
In `@cli/src/__tests__/lib/manifest.test.ts`:
- Around line 87-96: Add a test mirroring "supports overrides flag" to cover the
export field: call addToManifest(...) with an entry that includes export set to
a non-default value (e.g., "named"), then call readManifest(...) and assert the
returned entry's export equals that value; reference the existing helpers
addToManifest and readManifest and the behavior in runPluginAdd/ManifestEntry so
the test verifies the export field is persisted when it differs from "default".
- Around line 14-17: The temp directory name created in the beforeEach uses
Date.now() which can collide in parallel tests; update the setup to produce a
truly unique temp directory by replacing the join(tmpdir(), "neoboard-test-" +
Date.now()) logic with a UUID or mkdtemp-based creation (e.g., use
crypto.randomUUID() when constructing the directory name or call fs.mkdtempSync
with a "neoboard-test-" prefix) and keep the mkdirSync(tempDir, { recursive:
true }) or let mkdtempSync create the dir directly; edit the beforeEach that
assigns tempDir and the code using join/tmpdir/mkdirSync to ensure uniqueness.
In `@cli/src/__tests__/lib/plugin-validator.test.ts`:
- Around line 33-37: Add two tests to
cli/src/__tests__/lib/plugin-validator.test.ts that exercise the Array.isArray
branch and the ambiguous hasTransform && hasCreateModule branch in
validatePluginExport: one should call validatePluginExport([]) and assert
result.valid === false and that result.errors includes the "must be an object"
message (or the same message asserted for null), and the other should construct
an export object containing both transform and createModule properties, call
validatePluginExport(exported), assert result.valid === false and that
result.errors contains the specific ambiguity message emitted by
validatePluginExport (the message describing that both 'transform' and
'createModule' cannot be present).
In `@cli/src/lib/plugin-validator.ts`:
- Around line 50-67: The code duplicates the plugin-shape checks after calling
detectPluginType(obj); change detectPluginType (or add/export a small helper) so
it returns a tagged result you can switch on (e.g. "chart" | "connector" |
"ambiguous" | "none") or expose hasTransform/hasCreateModule helpers and use
those directly instead of re-evaluating; then replace the current pluginType
null branch in validate logic to use the tagged result (or the shared helpers)
to decide between the ambiguous vs missing error messages, ensuring the
shape-detection logic lives in one place (functions: detectPluginType,
hasTransform, hasCreateModule).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b1bf0672-22b5-4d74-b32d-3bbff871ab28
📒 Files selected for processing (6)
cli/src/__tests__/lib/manifest.test.tscli/src/__tests__/lib/plugin-validator.test.tscli/src/commands/plugin.tscli/src/index.tscli/src/lib/manifest.tscli/src/lib/plugin-validator.ts
| import { | ||
| validatePluginExport, | ||
| detectPluginType, | ||
| } from "../lib/plugin-validator.js"; |
There was a problem hiding this comment.
detectPluginType imported but unused.
ESLint is flagging this — validatePluginExport already returns pluginType via validation.pluginType, so the named import is dead.
🔧 Proposed fix
-import {
- validatePluginExport,
- detectPluginType,
-} from "../lib/plugin-validator.js";
+import { validatePluginExport } from "../lib/plugin-validator.js";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { | |
| validatePluginExport, | |
| detectPluginType, | |
| } from "../lib/plugin-validator.js"; | |
| import { validatePluginExport } from "../lib/plugin-validator.js"; |
🧰 Tools
🪛 ESLint
[error] 18-18: 'detectPluginType' is defined but never used.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/commands/plugin.ts` around lines 16 - 19, The import list in
plugin.ts includes detectPluginType but it is unused; remove detectPluginType
from the named imports so only validatePluginExport is imported
(validation.pluginType already provides the pluginType), e.g., update the import
statement to drop detectPluginType to satisfy ESLint and avoid dead code.
| const spinner = createSpinner("Installing " + packageName + "..."); | ||
| spinner.start(); | ||
| try { | ||
| run("npm install " + packageName, { cwd: root }); | ||
| spinner.succeed("Installed " + packageName); | ||
| } catch (err) { | ||
| spinner.fail("Failed to install " + packageName); | ||
| logError(String(err)); | ||
| process.exitCode = 1; | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the signature of `run` in exec.ts to know whether argv form is supported.
fd -t f 'exec.ts' cli/src
rg -nP -C5 'export\s+function\s+run\b|export\s+const\s+run\b' cli/src/lib/exec.tsRepository: alfredo1996/neoboard
Length of output: 590
🏁 Script executed:
#!/bin/bash
# Check the actual plugin.ts code around the flagged lines
fd -t f 'plugin.ts' cli/src/commands
head -80 cli/src/commands/plugin.ts | tail -60Repository: alfredo1996/neoboard
Length of output: 1787
🏁 Script executed:
#!/bin/bash
# Get the full run() function implementation to understand its behavior
cat -n cli/src/lib/exec.tsRepository: alfredo1996/neoboard
Length of output: 3229
Shell injection via packageName in run("npm install " + packageName, …) at lines 39, 254, 265.
packageName is user-controlled (CLI argument) and concatenated directly into a shell command string. The run() function uses execSync() without disabling shell interpretation, allowing commands like foo; rm -rf ~ to execute arbitrary shell code. The security comment in exec.ts stating "no user input is interpolated" does not match this usage.
Refactor to pass package name as a separate argument instead of string concatenation. Either:
- Add an argv form to
run()usingexecFileSyncwithshell: false, or - Call
execFileSyncdirectly in plugin.ts with command and args as separate parameters.
Apply the same fix to npm uninstall calls at lines 254 and 265, and to node <script> invocations if codegenScript is user-derived.
Also applies to: 252–257, 262–270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/commands/plugin.ts` around lines 36 - 46, The code concatenates
user-controlled packageName into a shell string passed to run(), causing shell
injection; change calls like run("npm install " + packageName, ...) to an
argv-style invocation so arguments are passed separately (e.g., run('npm',
['install', packageName], {cwd: root}) or call execFileSync('npm', ['install',
packageName'], {cwd: root, shell: false}) directly), and update run() in exec.ts
to support an argv form that uses execFileSync with shell: false; apply the same
refactor for the npm uninstall calls and any node <script> invocations that use
codegenScript (pass the script as a separate arg array rather than interpolating
it into a shell command).
| // 2. Try to load and validate the export | ||
| let exported: unknown; | ||
| try { | ||
| const mod = await import(packageName); | ||
| exported = | ||
| exportName === "default" ? (mod.default ?? mod) : mod[exportName]; | ||
| } catch (err) { | ||
| logError("Failed to import " + packageName + ": " + String(err)); | ||
| rollback(packageName, root); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# See how the CLI is packaged / resolved to gauge likelihood.
fd -t f 'package.json' cli -d 2 -x cat {}
rg -nP '"bin"\s*:' cli/package.jsonRepository: alfredo1996/neoboard
Length of output: 650
🏁 Script executed:
cat -n cli/src/commands/plugin.tsRepository: alfredo1996/neoboard
Length of output: 8453
🏁 Script executed:
# Check project structure and how CLI relates to main project
ls -la cli/
cat -n cli/src/lib/config.ts | head -50Repository: alfredo1996/neoboard
Length of output: 2136
🏁 Script executed:
# Look for tests related to plugin add
find cli -name '*.test.*' -o -name '*.spec.*' | head -20
rg -l "runPluginAdd" cli/Repository: alfredo1996/neoboard
Length of output: 917
🏁 Script executed:
# Check if plugin command is wired up in the CLI
grep -n "plugin" cli/src/index.tsRepository: alfredo1996/neoboard
Length of output: 721
Module resolution will fail when CLI is globally installed.
await import(packageName) resolves from the CLI's module context (cli/), but the package was installed in the user's project root via npm install with cwd: root. When the CLI is deployed globally or in a separate workspace, this import will fail with ERR_MODULE_NOT_FOUND, triggering spurious rollback/uninstall. Use createRequire(pathToFileURL(join(root, "package.json"))) to resolve the package from the user's project root instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/commands/plugin.ts` around lines 48 - 58, The dynamic ESM import of
packageName (await import(packageName)) fails when the CLI runs outside the
user's project; replace it by creating a require bound to the project root using
createRequire(pathToFileURL(join(root, "package.json"))) and use that require to
load the package so resolution happens from root, then set exported the same way
(exportName === "default" ? (mod.default ?? mod) : mod[exportName]); keep the
existing try/catch and call rollback(packageName, root) on error as before.
| const added = addToManifest( | ||
| manifestPath, | ||
| manifestKey as "plugins" | "connectors", | ||
| entry, | ||
| ); | ||
| if (!added) { | ||
| warn( | ||
| packageName + " is already registered in " + manifestFile + ". Skipping.", | ||
| ); | ||
| } | ||
|
|
||
| // 4. Run codegen | ||
| const codegenScript = | ||
| pluginType === "chart" | ||
| ? "scripts/generate-plugin-imports.mjs" | ||
| : "scripts/generate-connector-imports.mjs"; | ||
|
|
||
| try { | ||
| run("node " + codegenScript, { cwd: root }); | ||
| } catch { | ||
| warn("Codegen script failed. Run manually: node " + codegenScript); | ||
| } | ||
|
|
||
| success( | ||
| 'Plugin "' + | ||
| pluginLabel + | ||
| '" registered as ' + | ||
| pluginType + | ||
| " in " + | ||
| manifestFile, | ||
| ); |
There was a problem hiding this comment.
"Already registered" path still runs codegen and prints "…registered as chart".
When addToManifest returns false (duplicate package), the flow still falls through to codegen and then success('Plugin "…" registered as chart …'). That contradicts the preceding warn("… already registered … Skipping.") and can mask real problems (e.g. user thinks a re-add applied changes it didn’t). Return after the warn, or — since the spec wants --override to replace a conflict — gate behavior on overrides explicitly.
🔧 Proposed fix
if (!added) {
warn(
packageName + " is already registered in " + manifestFile + ". Skipping.",
);
+ return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const added = addToManifest( | |
| manifestPath, | |
| manifestKey as "plugins" | "connectors", | |
| entry, | |
| ); | |
| if (!added) { | |
| warn( | |
| packageName + " is already registered in " + manifestFile + ". Skipping.", | |
| ); | |
| } | |
| // 4. Run codegen | |
| const codegenScript = | |
| pluginType === "chart" | |
| ? "scripts/generate-plugin-imports.mjs" | |
| : "scripts/generate-connector-imports.mjs"; | |
| try { | |
| run("node " + codegenScript, { cwd: root }); | |
| } catch { | |
| warn("Codegen script failed. Run manually: node " + codegenScript); | |
| } | |
| success( | |
| 'Plugin "' + | |
| pluginLabel + | |
| '" registered as ' + | |
| pluginType + | |
| " in " + | |
| manifestFile, | |
| ); | |
| const added = addToManifest( | |
| manifestPath, | |
| manifestKey as "plugins" | "connectors", | |
| entry, | |
| ); | |
| if (!added) { | |
| warn( | |
| packageName + " is already registered in " + manifestFile + ". Skipping.", | |
| ); | |
| return; | |
| } | |
| // 4. Run codegen | |
| const codegenScript = | |
| pluginType === "chart" | |
| ? "scripts/generate-plugin-imports.mjs" | |
| : "scripts/generate-connector-imports.mjs"; | |
| try { | |
| run("node " + codegenScript, { cwd: root }); | |
| } catch { | |
| warn("Codegen script failed. Run manually: node " + codegenScript); | |
| } | |
| success( | |
| 'Plugin "' + | |
| pluginLabel + | |
| '" registered as ' + | |
| pluginType + | |
| " in " + | |
| manifestFile, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/commands/plugin.ts` around lines 98 - 128, The code continues to run
codegen and print a success message even when addToManifest(...) returns false
(duplicate) and warn(...) is called; update the flow in the command handler to
stop further processing for the duplicate case by returning early (or, if
behavior should allow replacing when an overrides flag is set, check the
overrides variable/option before proceeding) — specifically, after detecting
added === false for addToManifest(manifestPath, manifestKey, entry) call, either
return immediately (prevent run("node " + codegenScript, ...) and success(...))
or only continue when an explicit overrides option is true so codegenScript and
success('Plugin "' + pluginLabel + '" registered as ' + pluginType + ...) are
only executed for actual changes.
| export function runPluginList(): void { | ||
| const root = findProjectRoot(); | ||
|
|
||
| // Built-in chart types (hardcoded — mirrors chart-types.ts) | ||
| const builtInCharts = [ | ||
| "bar", | ||
| "line", | ||
| "pie", | ||
| "table", | ||
| "single-value", | ||
| "graph", | ||
| "map", | ||
| "json", | ||
| "parameter-select", | ||
| "form", | ||
| "markdown", | ||
| "iframe", | ||
| "gauge", | ||
| "sankey", | ||
| "sunburst", | ||
| "radar", | ||
| "treemap", | ||
| "gantt", | ||
| ]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the canonical built-in chart/connector lists to import from.
fd -t f 'chart-types.ts'
rg -nP 'export\s+const\s+(BUILT_IN_CHARTS|CHART_TYPES|BUILTIN_CHARTS)\b' -C2
rg -nP '\bCONNECTOR_TYPES\b' connection/src -n -C2Repository: alfredo1996/neoboard
Length of output: 1068
🏁 Script executed:
# Read the full CHART_TYPES export from the canonical source
cat -n app/src/plugins/chart-types.tsRepository: alfredo1996/neoboard
Length of output: 882
🏁 Script executed:
# Read the context around the hardcoded list in plugin.ts
sed -n '130,160p' cli/src/commands/plugin.tsRepository: alfredo1996/neoboard
Length of output: 631
🏁 Script executed:
# Check if there are other hardcoded chart lists elsewhere
rg -n 'bar.*line.*pie' --type ts --type tsx -C 2Repository: alfredo1996/neoboard
Length of output: 92
🏁 Script executed:
# Verify the import path would be valid from cli to app
# (Check relative path structure)
git ls-files cli/src/commands/ app/src/plugins/ | head -20Repository: alfredo1996/neoboard
Length of output: 821
🏁 Script executed:
# Verify exact relative path from cli to app for import statement
# Count directory depth
echo "cli path: cli/src/commands/plugin.ts"
echo "app path: app/src/plugins/chart-types.ts"
# From cli/src/commands/ need to go: ../../.. (to root) then app/src/plugins/
# Relative would be: ../../../app/src/plugins/chart-types.ts
# Also verify if "gantt" is mentioned elsewhere as a valid chart type
rg -n '"gantt"' --type ts -C 2Repository: alfredo1996/neoboard
Length of output: 344
Fix hardcoded chart list—already drifted from canonical source.
The hardcoded builtInCharts array duplicates CHART_TYPES from app/src/plugins/chart-types.ts and is out of sync: "gantt" appears here but not in the canonical registry. Import the source of truth instead:
import { CHART_TYPES } from "../../../app/src/plugins/chart-types";Same issue affects builtInConnectors—import CONNECTOR_TYPES from connection/src/connector-types instead of hardcoding.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/commands/plugin.ts` around lines 134 - 157, Replace the hardcoded
built-in lists in runPluginList: remove the local builtInCharts and
builtInConnectors arrays and import the canonical CHART_TYPES and
CONNECTOR_TYPES arrays from their source modules, then use CHART_TYPES and
CONNECTOR_TYPES in place of builtInCharts/builtInConnectors; ensure you add the
corresponding imports at the top of the file and update any references inside
runPluginList to use CHART_TYPES and CONNECTOR_TYPES (preserve any
formatting/filtering logic currently applied to the hardcoded arrays).
| try { | ||
| const raw = JSON.parse(readFileSync(filePath, "utf-8")); | ||
| return Array.isArray(raw[key]) ? raw[key] : []; | ||
| } catch { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
readManifest returns unvalidated data cast as ManifestEntry[].
If the JSON is malformed (e.g. { "plugins": [1, 2, null] }), these non-objects flow into callers that access e.package (e.g. runPluginList at plugin.ts line 180, addToManifest dedup at line 41), producing confusing crashes or bogus undefined rows. Consider filtering to entries where typeof e === "object" && e && typeof e.package === "string".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/lib/manifest.ts` around lines 23 - 28, readManifest currently returns
raw array items as ManifestEntry[] which lets malformed entries
(numbers/null/objects missing package) reach callers like runPluginList and
addToManifest; update readManifest to validate and filter the parsed array so it
only returns entries where typeof e === "object" && e !== null && typeof (e as
any).package === "string" (and any other required fields), then map/cast to
ManifestEntry[] before returning to prevent undefined package accesses and bogus
rows.
| export function addToManifest( | ||
| filePath: string, | ||
| key: ManifestKey, | ||
| entry: ManifestEntry, | ||
| ): boolean { | ||
| const entries = readManifest(filePath, key); | ||
| if (entries.some((e) => e.package === entry.package)) { | ||
| return false; // already exists | ||
| } | ||
| entries.push(entry); | ||
| writeFileSync(filePath, JSON.stringify({ [key]: entries }, null, 2) + "\n"); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Writes drop the $schema property (and any other top-level keys).
The existing neoboard-plugins.json at the repo root is { "$schema": "./neoboard-plugins.schema.json", "plugins": [] } (per the schema context). addToManifest/removeFromManifest serialize only { [key]: … }, so after the first plugin add the $schema reference is silently stripped, breaking IDE validation and drifting from the committed file format.
Preserve the existing object and only update the target key.
🔧 Proposed fix
export function addToManifest(
filePath: string,
key: ManifestKey,
entry: ManifestEntry,
): boolean {
- const entries = readManifest(filePath, key);
+ const existing = existsSync(filePath)
+ ? (() => {
+ try {
+ return JSON.parse(readFileSync(filePath, "utf-8")) as Record<string, unknown>;
+ } catch {
+ return {};
+ }
+ })()
+ : {};
+ const entries = Array.isArray(existing[key])
+ ? (existing[key] as ManifestEntry[])
+ : [];
if (entries.some((e) => e.package === entry.package)) {
return false;
}
entries.push(entry);
- writeFileSync(filePath, JSON.stringify({ [key]: entries }, null, 2) + "\n");
+ writeFileSync(
+ filePath,
+ JSON.stringify({ ...existing, [key]: entries }, null, 2) + "\n",
+ );
return true;
}Apply the analogous change in removeFromManifest.
Also applies to: 53-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/lib/manifest.ts` around lines 35 - 47, addToManifest (and the
analogous removeFromManifest) currently overwrite the entire manifest with only
{ [key]: entries }, dropping top-level keys like "$schema"; instead read and
parse the existing JSON object (using the existing readManifest helper to get
entries but also JSON.parse the file), replace only the target property (e.g.,
manifest[key] = entries or updated entries), and write the full manifest object
back with JSON.stringify(...) to preserve other top-level keys (ensure
writeFileSync is still used and keep pretty-printing and trailing newline).
| if (pluginType === "chart") { | ||
| if (!Array.isArray(obj.compatibleWith) || obj.compatibleWith.length === 0) { | ||
| errors.push( | ||
| '"compatibleWith" must be a non-empty array of connector types', | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing component field validation and untyped compatibleWith entries.
Per the PR/issue spec and ChartPluginConfig in app/src/lib/plugin/chart-plugin-registry.ts, chart plugins must provide a React component. The validator currently accepts a chart plugin with no component, so a malformed chart export can pass, be registered, then crash at render time.
Additionally, compatibleWith length is checked but entry types are not, so compatibleWith: [123, null] passes validation and breaks the registry.
🔧 Proposed fix
if (pluginType === "chart") {
+ if (typeof obj.component !== "function" && typeof obj.component !== "object") {
+ errors.push('"component" must be a React component');
+ }
if (!Array.isArray(obj.compatibleWith) || obj.compatibleWith.length === 0) {
errors.push(
'"compatibleWith" must be a non-empty array of connector types',
);
+ } else if (!obj.compatibleWith.every((c) => typeof c === "string" && c.length > 0)) {
+ errors.push('"compatibleWith" entries must be non-empty strings');
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (pluginType === "chart") { | |
| if (!Array.isArray(obj.compatibleWith) || obj.compatibleWith.length === 0) { | |
| errors.push( | |
| '"compatibleWith" must be a non-empty array of connector types', | |
| ); | |
| } | |
| } | |
| if (pluginType === "chart") { | |
| if (typeof obj.component !== "function" && typeof obj.component !== "object") { | |
| errors.push('"component" must be a React component'); | |
| } | |
| if (!Array.isArray(obj.compatibleWith) || obj.compatibleWith.length === 0) { | |
| errors.push( | |
| '"compatibleWith" must be a non-empty array of connector types', | |
| ); | |
| } else if (!obj.compatibleWith.every((c) => typeof c === "string" && c.length > 0)) { | |
| errors.push('"compatibleWith" entries must be non-empty strings'); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/lib/plugin-validator.ts` around lines 70 - 76, The chart plugin
validator currently only checks compatibleWith length and misses verifying a
required React component; update the plugin validation inside the pluginType ===
"chart" branch (the block that references obj.compatibleWith) to also ensure
obj.component is present and is a callable/component type (e.g., a function or
object consistent with React.ComponentType) and to validate each entry of
obj.compatibleWith is a non-empty string (reject numbers/null/other types). Add
clear error messages to errors.push when component is missing/invalid and when
any compatibleWith entry is not a string or is empty.
|
2 participants
Summary
New CLI commands for managing external NeoBoard plugins without manually editing JSON manifests.
Commands
Validation Pipeline
After
npm install, the CLI validates the package before registering:type(string),label(string)transformfunction → chart,createModulefunction → connectorcompatibleWithmust be a non-empty arraycategorymust be one ofdatabase,graph,api,fileFiles
cli/src/commands/plugin.ts— add, list, remove implementationscli/src/lib/plugin-validator.ts— export validationcli/src/lib/manifest.ts— read/write/remove manifest entriescli/src/index.ts— registerplugincommand groupTest plan
Closes #605
🤖 Generated with Claude Code
Summary by CodeRabbit
pluginCLI command group to manage external plugins withadd,list, andremovesubcommandsaddcommand supports--overrideand--exportflags for flexible installationlistsubcommand displays built-in and external plugins/connectors with override indicators