feat(plugins): external chart plugin loading via manifest (#423) - #589
Merged
Conversation
Closes #423 — Phase 1. New: neoboard-plugins.json - JSON manifest at repo root lists external npm packages that export ChartPlugin definitions - JSON schema (neoboard-plugins.schema.json) for editor autocompletion - Entries: { package, export?, overrides? } New: scripts/generate-plugin-imports.mjs - Runs at predev + prebuild - Validates the manifest, generates app/src/plugins/external-plugins.generated.ts with static import statements - Idempotent — skips writing when content is unchanged - Rejects duplicate package+export pairs, unknown keys, malformed JSON Plugin bootstrap (app/src/plugins/index.ts) - Imports the generated EXTERNAL_PLUGINS array after built-ins - Same-type conflicts throw loudly unless the manifest entry sets "overrides": true (explicit opt-in, never silent) Trust model - External plugins run in-process with full app access — same trust as any npm dep bundled at build time - Adding a plugin requires repo commit + npm install; there is no runtime plugin surface - Documented in docs/plugins/authoring.md Tests - 25 generator tests (validateEntry/validateManifest/renderSource/ runGenerator, temp-dir e2e, idempotent write, invalid JSON, duplicates) - 7 bootstrap tests (unique register, conflict rejection, overrides replace, batch-abort-on-first-conflict, end-to-end real registry) - Full suite: 2097 pass Deferred to follow-ups: - Example plugin workspace (examples/plugin-sparkline/) - Plugin SDK re-export package (right now plugins import from app/src/lib — SDK package will stabilize the public surface) Closes #423 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 54 minutes and 10 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughA build-time manifest-driven external plugin system: a JSON manifest and schema define external plugins; a Node generator produces Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (build)
participant FS as File System / Generator
participant Manifest as neoboard-plugins.json
participant Build as Build process (predev/prebuild)
participant App as App runtime
participant Registry as pluginRegistry
Dev->>FS: edit `neoboard-plugins.json`
Build->>FS: runs `generate-plugin-imports.mjs`
FS->>Manifest: read & validate manifest
FS->>FS: render `external-plugins.generated.ts` (idempotent)
FS->>File System: write generated file if changed
Build->>App: start dev/build (imports generated file)
App->>Registry: register built-in plugins
App->>Registry: iterate EXTERNAL_PLUGINS
alt plugin.type exists and overrides == false
Registry-->>App: throw startup error (conflict)
else plugin.type exists and overrides == true
Registry->>Registry: unregister existing
Registry->>Registry: register external plugin
else
Registry->>Registry: register external plugin
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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: 6
🧹 Nitpick comments (2)
app/src/plugins/__tests__/external-plugins-bootstrap.test.ts (1)
24-45: Test the production registration helper instead of mirroring it.This duplicates the loop from
plugins/index.ts, so the tests can pass even if the real bootstrap logic drifts. Consider extracting the external-registration loop into a pure helper and importing that here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/plugins/__tests__/external-plugins-bootstrap.test.ts` around lines 24 - 45, The test currently duplicates the external-plugin registration loop (function registerExternalPlugins) instead of using the production helper; remove this duplicated function and instead import and call the single pure helper used by the runtime (extract it from plugins/index.ts into a small pure function if it doesn't yet exist), then update the tests to call that helper with the same signature (accepting the registry from createPluginRegistry and entries array of { plugin, overrides }) so the test exercises the real registration logic and uses registry.has, registry.unregister and registry.register from the production implementation.scripts/__tests__/generate-plugin-imports.test.mjs (1)
48-67: Add tests for special characters in package and export names.The
renderSourcetests only validate simple strings like"@a/one"and"heatmap". Sincepackageandexportare rendered directly into import statements without escaping (lines 143, 145 in the generator), add regression cases for quotes, newlines, and invalid identifier syntax to ensure malformed TypeScript cannot be generated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/__tests__/generate-plugin-imports.test.mjs` around lines 48 - 67, Add regression tests to prevent unescaped special characters in generated import statements: extend the test suite (the same file with validateEntry and renderSource coverage) to include cases where package and export contain quotes, newlines, and invalid identifier characters (e.g., package: '"bad"', '"pkg\n"', export: 'weird-name!', etc.). Use validateEntry to assert these produce validation errors (or that renderSource refuses/throws) by matching messages indicating invalid/unsafe package or export values; reference validateEntry and renderSource so reviewers can locate and add tests that ensure the generator does not emit unescaped/malformed TypeScript imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/plugins/authoring.md`:
- Line 59: The fenced code block in the docs/plugins/authoring.md currently
lacks a language tag; update the opening fence for the directory tree (the block
containing "my-neoboard-plugin/") to use the text language (change the
triple-backtick fence to ```text) so the Markdown linter stops flagging it and
the tree renders as plain text.
In `@neoboard-plugins.schema.json`:
- Around line 17-24: The schema lets "package" and "export" be arbitrary
strings, which permits whitespace and invalid JS identifiers that will break
generated TypeScript; update the neoboard-plugins.schema.json definitions for
"package" to include a "pattern" that forbids whitespace and matches valid
import specifiers (e.g. allow scoped packages, relative paths and characters
typically valid in package names like letters, digits, -, _, ., / and @) and for
"export" to include a "pattern" that enforces valid JS export names (e.g.
^[A-Za-z_$][A-Za-z0-9_$]*$) or the literal "default"; keep the existing
"minLength" checks and add concise regex patterns on the "package" and "export"
properties so editors validate manifests the same way the generator expects.
In `@package.json`:
- Around line 13-18: The test and lint scripts currently don't generate
external-plugins.generated.ts (which app/src/plugins/index.ts imports) before
running, causing CI/local `npm test` or `npm run lint` to fail; add pretest,
pretest:app, and prelint scripts (or update existing test scripts) in
package.json to run "node scripts/generate-plugin-imports.mjs" so generation
runs before test/lint tasks and ensures external-plugins.generated.ts exists.
In `@scripts/__tests__/generate-plugin-imports.test.mjs`:
- Around line 196-207: Replace the current idempotency assertion that only
checks second.wrote with a filesystem mtime check: after the first runGenerator
call (in the "is idempotent" test using withTempDir), record the output file's
mtime via fs.statSync (or fs.promises.stat) and then run the generator a second
time; assert that second.wrote is false and that the output file's mtime is
unchanged (compare the stat.mtime value from before and after). Use the existing
runGenerator, readFileSync and output path variables to locate the file and
fs.utimesSync only if you need to set a known mtime for a deterministic
comparison. Ensure the test still reads the file contents to confirm content
equality as before.
In `@scripts/generate-plugin-imports.mjs`:
- Around line 76-84: validateManifest currently only checks that raw is an
object and that m.plugins is an array, but it doesn't reject unknown top-level
keys contrary to the schema's additionalProperties:false; update
validateManifest to detect any top-level keys other than "plugins" (e.g., using
Object.keys(m) and filtering out "plugins"), push a descriptive error like
"unknown top-level manifest keys: X" into the errors array when extras exist,
and return the error result (same shape as the other early returns) so manifests
with extra keys are rejected; reference the validateManifest function and the
local variable m/plugins when making the change.
- Around line 43-65: The validateEntry function must enforce safe module
specifiers and export identifiers before code generation: add regex checks in
validateEntry to reject package strings containing whitespace, quotes (" or '),
backslashes, or other unsafe chars (e.g., /^[^\s"\'\\]+$/) and validate e.export
when present matches a valid JS identifier (e.g., /^[A-Za-z_$][A-Za-z0-9_$]*$/);
return a descriptive error message on failure. Then, when emitting import code
for e.package and e.export (where template interpolation currently occurs),
ensure you use escaped/serialized values (e.g., JSON.stringify(e.package)) for
the module specifier and only emit a named export if e.export passed validation.
Also ensure overrides handling still respects the allowed keys check in
validateEntry.
---
Nitpick comments:
In `@app/src/plugins/__tests__/external-plugins-bootstrap.test.ts`:
- Around line 24-45: The test currently duplicates the external-plugin
registration loop (function registerExternalPlugins) instead of using the
production helper; remove this duplicated function and instead import and call
the single pure helper used by the runtime (extract it from plugins/index.ts
into a small pure function if it doesn't yet exist), then update the tests to
call that helper with the same signature (accepting the registry from
createPluginRegistry and entries array of { plugin, overrides }) so the test
exercises the real registration logic and uses registry.has, registry.unregister
and registry.register from the production implementation.
In `@scripts/__tests__/generate-plugin-imports.test.mjs`:
- Around line 48-67: Add regression tests to prevent unescaped special
characters in generated import statements: extend the test suite (the same file
with validateEntry and renderSource coverage) to include cases where package and
export contain quotes, newlines, and invalid identifier characters (e.g.,
package: '"bad"', '"pkg\n"', export: 'weird-name!', etc.). Use validateEntry to
assert these produce validation errors (or that renderSource refuses/throws) by
matching messages indicating invalid/unsafe package or export values; reference
validateEntry and renderSource so reviewers can locate and add tests that ensure
the generator does not emit unescaped/malformed TypeScript imports.
🪄 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: ff3dee81-92c7-42d9-aca2-9dbe6cb9e539
📒 Files selected for processing (9)
.gitignoreapp/src/plugins/__tests__/external-plugins-bootstrap.test.tsapp/src/plugins/index.tsdocs/plugins/authoring.mdneoboard-plugins.jsonneoboard-plugins.schema.jsonpackage.jsonscripts/__tests__/generate-plugin-imports.test.mjsscripts/generate-plugin-imports.mjs
|
|
||
| Your plugin should be an npm package. Minimum structure: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add a language to the package-layout fence.
Markdown lint flags this fenced block; use text for the directory tree.
Proposed fix
-```
+```text
my-neoboard-plugin/📝 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.
Suggested change
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 59-59: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plugins/authoring.md` at line 59, The fenced code block in the
docs/plugins/authoring.md currently lacks a language tag; update the opening
fence for the directory tree (the block containing "my-neoboard-plugin/") to use
the text language (change the triple-backtick fence to ```text) so the Markdown
linter stops flagging it and the tree renders as plain text.
Comment on lines
+17
to
+24
| "package": { | ||
| "type": "string", | ||
| "minLength": 1, | ||
| "description": "The npm package name to import (e.g. '@myorg/neoboard-heatmap' or './plugins/my-local')." | ||
| }, | ||
| "export": { | ||
| "type": "string", | ||
| "minLength": 1, |
There was a problem hiding this comment.
Constrain manifest strings to values codegen can import.
The schema currently accepts whitespace and invalid named exports that will later generate broken TypeScript. Mirror the generator’s stricter rules here so editors catch bad manifests early.
Proposed schema tightening
"package": {
"type": "string",
"minLength": 1,
+ "pattern": "^\\S+$",
"description": "The npm package name to import (e.g. '@myorg/neoboard-heatmap' or './plugins/my-local')."
},
"export": {
"type": "string",
"minLength": 1,
+ "pattern": "^[A-Za-z_$][A-Za-z0-9_$]*$",
"default": "default",
"description": "Named export to import. Defaults to 'default' (the package's default export)."📝 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.
Suggested change
| "package": { | |
| "type": "string", | |
| "minLength": 1, | |
| "description": "The npm package name to import (e.g. '@myorg/neoboard-heatmap' or './plugins/my-local')." | |
| }, | |
| "export": { | |
| "type": "string", | |
| "minLength": 1, | |
| "package": { | |
| "type": "string", | |
| "minLength": 1, | |
| "pattern": "^\\S+$", | |
| "description": "The npm package name to import (e.g. '@myorg/neoboard-heatmap' or './plugins/my-local')." | |
| }, | |
| "export": { | |
| "type": "string", | |
| "minLength": 1, | |
| "pattern": "^[A-Za-z_$][A-Za-z0-9_$]*$", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@neoboard-plugins.schema.json` around lines 17 - 24, The schema lets "package"
and "export" be arbitrary strings, which permits whitespace and invalid JS
identifiers that will break generated TypeScript; update the
neoboard-plugins.schema.json definitions for "package" to include a "pattern"
that forbids whitespace and matches valid import specifiers (e.g. allow scoped
packages, relative paths and characters typically valid in package names like
letters, digits, -, _, ., / and @) and for "export" to include a "pattern" that
enforces valid JS export names (e.g. ^[A-Za-z_$][A-Za-z0-9_$]*$) or the literal
"default"; keep the existing "minLength" checks and add concise regex patterns
on the "package" and "export" properties so editors validate manifests the same
way the generator expects.
Comment on lines
+196
to
+207
| it("is idempotent — second run with unchanged manifest does not rewrite", () => { | ||
| withTempDir(({ manifest, output }) => { | ||
| writeFileSync(manifest, JSON.stringify({ plugins: [] })); | ||
| runGenerator({ manifestPath: manifest, outputPath: output }); | ||
| const first = readFileSync(output, "utf8"); | ||
| const second = runGenerator({ | ||
| manifestPath: manifest, | ||
| outputPath: output, | ||
| }); | ||
| assert.equal(second.wrote, false); | ||
| assert.equal(readFileSync(output, "utf8"), first); | ||
| }); |
There was a problem hiding this comment.
Assert the file was not rewritten.
This currently trusts second.wrote; it would still pass if the generator rewrote identical content but returned false. Set a known mtime after the first run and assert it is unchanged after the second run.
Proposed test hardening
-import { mkdtempSync, writeFileSync, readFileSync, existsSync } from "node:fs";
+import {
+ mkdtempSync,
+ writeFileSync,
+ readFileSync,
+ existsSync,
+ statSync,
+ utimesSync,
+} from "node:fs";
@@
writeFileSync(manifest, JSON.stringify({ plugins: [] }));
runGenerator({ manifestPath: manifest, outputPath: output });
+ const untouchedTimestamp = new Date("2000-01-01T00:00:00.000Z");
+ utimesSync(output, untouchedTimestamp, untouchedTimestamp);
const first = readFileSync(output, "utf8");
+ const firstMtimeMs = statSync(output).mtimeMs;
const second = runGenerator({
manifestPath: manifest,
outputPath: output,
});
assert.equal(second.wrote, false);
assert.equal(readFileSync(output, "utf8"), first);
+ assert.equal(statSync(output).mtimeMs, firstMtimeMs);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/__tests__/generate-plugin-imports.test.mjs` around lines 196 - 207,
Replace the current idempotency assertion that only checks second.wrote with a
filesystem mtime check: after the first runGenerator call (in the "is
idempotent" test using withTempDir), record the output file's mtime via
fs.statSync (or fs.promises.stat) and then run the generator a second time;
assert that second.wrote is false and that the output file's mtime is unchanged
(compare the stat.mtime value from before and after). Use the existing
runGenerator, readFileSync and output path variables to locate the file and
fs.utimesSync only if you need to set a known mtime for a deterministic
comparison. Ensure the test still reads the file contents to confirm content
equality as before.
Comment on lines
+76
to
+84
| export function validateManifest(raw) { | ||
| const errors = []; | ||
| if (typeof raw !== "object" || raw === null) { | ||
| return { errors: ["manifest must be a JSON object"], entries: [] }; | ||
| } | ||
| const m = /** @type {Record<string, unknown>} */ (raw); | ||
| if (!Array.isArray(m.plugins)) { | ||
| return { errors: ["manifest.plugins must be an array"], entries: [] }; | ||
| } |
There was a problem hiding this comment.
Reject unknown top-level manifest keys too.
The schema has additionalProperties: false, but the generator only validates plugins. A manifest with extra top-level keys will pass codegen while editors/schema validation reject it.
Proposed fix
}
const m = /** `@type` {Record<string, unknown>} */ (raw);
+ const allowedTopLevel = new Set(["$schema", "plugins"]);
+ for (const key of Object.keys(m)) {
+ if (!allowedTopLevel.has(key)) {
+ errors.push(`manifest has unknown key "${key}"`);
+ }
+ }
if (!Array.isArray(m.plugins)) {
return { errors: ["manifest.plugins must be an array"], entries: [] };
}
+ if (errors.length > 0) {
+ return { errors, entries: [] };
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/generate-plugin-imports.mjs` around lines 76 - 84, validateManifest
currently only checks that raw is an object and that m.plugins is an array, but
it doesn't reject unknown top-level keys contrary to the schema's
additionalProperties:false; update validateManifest to detect any top-level keys
other than "plugins" (e.g., using Object.keys(m) and filtering out "plugins"),
push a descriptive error like "unknown top-level manifest keys: X" into the
errors array when extras exist, and return the error result (same shape as the
other early returns) so manifests with extra keys are rejected; reference the
validateManifest function and the local variable m/plugins when making the
change.
Dockerfile does 'cd app && npm run build', bypassing the root prebuild hook. Adding predev/prebuild to app/package.json that invokes the root codegen script ensures the generated file exists regardless of entry point. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The TypeScript type-check CI job runs tsc directly without invoking prebuild, so a gitignored generated file breaks type-check. Commit the generated file (empty by default) and let predev/prebuild regenerate it when the manifest changes. Codegen is idempotent, so no-op writes don't dirty git. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
4 tasks
This was referenced Apr 24, 2026
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
2 participants
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.
Summary
Phase 1 of #423 — external chart plugins can now be loaded from npm packages via a build-time manifest.
Manifest
neoboard-plugins.jsonat repo root lists external packages that exportChartPlugindefinitionsneoboard-plugins.schema.json) drives editor autocompletion{ package, export?, overrides? }Build-time codegen
scripts/generate-plugin-imports.mjsruns aspredev+prebuildapp/src/plugins/external-plugins.generated.tswith staticimportstatementspackage, unknown keys, duplicate package+export pairsBootstrap
app/src/plugins/index.tsimports the generatedEXTERNAL_PLUGINSarray after built-ins"overrides": true— explicit opt-in, never silentTrust model (documented in
docs/plugins/authoring.md)ChartErrorBoundary(already landed in fix(hardening): dashboard error boundary + expression tokenizer #576) catches per-widget crashesTest plan
Deferred to follow-ups
examples/plugin-sparkline/) — a copy-paste starter. Keeping it out of this PR to keep the diff reviewable.@neoboard/plugin-sdk) — stable public re-export ofdefineChartPlugin+ types. Right now plugins import fromapp/src/lib/plugin/…which is fragile.Closes #423
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores