feat(connectors): pluggable connector system with manifest discovery - #604
Merged
Merged
Conversation
…603) Mirror the chart plugin pattern for connectors: external database connectors can now be installed as npm packages and registered via a neoboard-connectors.json manifest without forking the repo. Infrastructure: - neoboard-connectors.json manifest (same shape as neoboard-plugins.json) - scripts/generate-connector-imports.mjs codegen (18 tests) - connection/src/external-connectors.generated.ts auto-generated - connector-registry.ts loads EXTERNAL_CONNECTORS after built-ins - Conflict detection: same-type without overrides:true throws at startup - predev/prebuild scripts run both plugin + connector codegen Manifest format: { "connectors": [{ "package": "@myorg/mongodb", "overrides": false }] } External connector npm package exports a ConnectorPlugin: { type, label, category, queryLanguage, createModule(), formFields, ... } Closes #603 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 39 minutes and 57 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 (1)
WalkthroughThis PR implements a pluggable connector system with manifest-based discovery. A new code generator reads Changes
Sequence Diagram(s)sequenceDiagram
participant manifest as neoboard-connectors.json
participant generator as generate-connector-imports.mjs
participant output as external-connectors.generated.ts
participant pkg as package.json hooks
pkg->>generator: predev/prebuild execute
generator->>manifest: read & validate
manifest-->>generator: entries array
generator->>generator: validate schema<br/>(package, export, overrides)
generator->>generator: detect duplicates
generator->>generator: render imports & array
generator->>output: write typed source
output-->>generator: ✓ generated
sequenceDiagram
participant registry as connector-registry<br/>(module init)
participant external as EXTERNAL_CONNECTORS
participant builtins as Built-in plugins
participant storage as Registry storage
registry->>builtins: register built-ins first
builtins-->>storage: ✓ registered
registry->>external: import & iterate
external-->>registry: [ExternalConnectorEntry, ...]
loop for each external connector
registry->>storage: check type exists?
alt type exists && no overrides
registry->>registry: ✗ throw startup error
else type exists && overrides
registry->>storage: unregister old type
storage-->>registry: ✓ removed
end
registry->>storage: register external plugin
storage-->>registry: ✓ registered
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 1
🧹 Nitpick comments (5)
connection/src/connector-registry.ts (1)
2-11: Header JSDoc is stale — mention the manifest path.The top-of-file docblock only describes the "import and register it here" workflow, but the primary path for external connectors is now
neoboard-connectors.json+ codegen. Add a bullet so operators don't miss it.📝 Suggested tweak
/** * Global connector registry — singleton that auto-registers built-in * connectors (Neo4j, PostgreSQL) on first import. * - * To add a new connector: - * 1. Create a plugin file implementing ConnectorPlugin - * 2. Import and register it here + * To add a new connector: + * 1. Built-in: implement ConnectorPlugin and register() it below. + * 2. External: publish a package exporting a ConnectorPlugin, list it + * in neoboard-connectors.json, and let `npm run generate:connectors` + * wire it into EXTERNAL_CONNECTORS. * * External/community connectors can call registerConnector() from * their own package after importing this module. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connection/src/connector-registry.ts` around lines 2 - 11, Update the header JSDoc in connector-registry.ts to mention the new manifest/codegen workflow: add a bullet that external/community connectors can be discovered via the neoboard-connectors.json manifest (used by the codegen pipeline) in addition to calling registerConnector() at runtime, and reference the ConnectorPlugin interface and registerConnector function so operators know which shape and API the manifest/codegen must produce.scripts/__tests__/generate-connector-imports.test.mjs (2)
40-44: Nit: test name says "invalid export identifier" but asserts on identifier syntax.Minor readability: the message in the error is
"must be a valid JavaScript identifier". Could tighten by also asserting a case like{ export: "123abc" }(leading digit) to make the intent crisper. Optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/__tests__/generate-connector-imports.test.mjs` around lines 40 - 44, Update the test for validateEntry to better reflect and assert identifier-syntax failures: rename the spec string from "rejects invalid export identifier" to something like "rejects invalid export identifier syntax" and add an explicit assertion that validateEntry({ package: "pkg", export: "123abc" }, 0) returns the error message containing "must be a valid JavaScript identifier" (in addition to the existing "not-valid" case) so the intent and a clear leading-digit identifier failure are both covered.
99-128: Consider a multi-entryrenderSourcecase and arunGeneratorsmoke test.Current tests nail the branches of the pure helpers. A couple of additions would catch likely future regressions:
renderSourcewith 2+ entries → asserts newline joining and that aliases increment (externalConnector0,externalConnector1).runGeneratorwith a tmp dir: (a) missing manifest → empty file written, (b) second run →wrote: false(idempotency), (c) malformed JSON →ok: falsewith useful error.validateEntryrejecting non-booleanoverridesand non-stringexport.Not blocking — the 18 cases already pin the critical schema rules.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/__tests__/generate-connector-imports.test.mjs` around lines 99 - 128, Add tests that cover multi-entry and generator/validation edge cases: for renderSource, add a test invoking renderSource with 2+ entries and assert the output contains both aliases (externalConnector0 and externalConnector1) and that the imports/entries are separated by a newline; for runGenerator, add a smoke test using a temp directory that verifies (a) when manifest is missing an empty file is written, (b) a second run reports wrote: false (idempotency), and (c) when the manifest contains malformed JSON the result is ok: false and returns a useful error; and for validateEntry add tests that ensure it rejects non-boolean overrides and non-string export values. Reference the existing test suite/helpers renderSource, runGenerator, and validateEntry when adding these cases.scripts/generate-connector-imports.mjs (2)
248-252: Log path breaks on Windows.
OUTPUT_PATH.replace(REPO_ROOT + "/", "")hard-codes a forward slash. On Windows the separator is\, so the replace is a no-op and the log prints an absolute path. Cosmetic only, but trivial to fix.🪟 Suggested fix
-import { dirname, resolve } from "node:path"; +import { dirname, relative, resolve } from "node:path"; @@ - console.log( - `Generated ${OUTPUT_PATH.replace(REPO_ROOT + "/", "")} from manifest`, - ); + console.log( + `Generated ${relative(REPO_ROOT, OUTPUT_PATH)} from manifest`, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/generate-connector-imports.mjs` around lines 248 - 252, The log prints an absolute path on Windows because OUTPUT_PATH.replace(REPO_ROOT + "/", "") hard-codes "/" as the separator; change the logging to compute a platform-safe relative path (e.g., using path.relative(REPO_ROOT, OUTPUT_PATH)) before printing so the console.log in scripts/generate-connector-imports.mjs (the block checking result.wrote) outputs a repo-relative path cross-platform; update the reference to use that relative path variable instead of the string replace.
189-200: Consider a quiet-but-visible note when the manifest is absent.Silently generating an empty file is the documented behavior, but if a contributor deletes
neoboard-connectors.jsonby accident their external connectors vanish with zero signal. A singleconsole.warn(stillok: true) would surface the situation without failing the build. Take it or leave it — the current behavior matches the PR spec.🗣️ Optional nudge
if (!existsSync(manifestPath)) { // No manifest = no external connectors. Generate empty file silently. + if (opts.manifestPath === undefined) { + console.warn( + `[generate-connector-imports] ${relative(REPO_ROOT, manifestPath)} not found; emitting empty EXTERNAL_CONNECTORS.`, + ); + } const source = renderSource([]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/generate-connector-imports.mjs` around lines 189 - 200, When manifestPath is missing the function currently silently writes an empty connector output; update the branch that handles !existsSync(manifestPath) to emit a single visible warning (e.g., console.warn) indicating the manifest is absent and external connectors will not be generated while preserving the current return values ({ ok: true, errors: [], wrote: ...}). Locate the block that calls renderSource([]), checks existing via existsSync(outputPath) / readFileSync, and writes via writeFileSync, and insert the console.warn before returning so the behavior of renderSource, existing/outputPath checks, and writeFileSync remains unchanged except for the added warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@connection/src/connector-registry.ts`:
- Around line 31-44: The error message thrown when an external connector
conflicts currently hard-codes "built-in" and can be misleading for
external-vs-external clashes; update the logic around EXTERNAL_CONNECTORS so
when registry.has(plugin.type) you detect whether the existing entry came from a
built-in or another external source and throw a clearer error: if overrides is
false, throw an error referencing plugin.type and saying the connector conflicts
with an existing connector (specifying "built-in" only when the existing entry
is a built-in, otherwise indicate "another external connector"), and include
guidance to set "overrides": true in neoboard-connectors.json when appropriate;
use the existing symbols (EXTERNAL_CONNECTORS, plugin.type, overrides,
registry.has, registry.unregister, registry.register) to locate and change the
message.
---
Nitpick comments:
In `@connection/src/connector-registry.ts`:
- Around line 2-11: Update the header JSDoc in connector-registry.ts to mention
the new manifest/codegen workflow: add a bullet that external/community
connectors can be discovered via the neoboard-connectors.json manifest (used by
the codegen pipeline) in addition to calling registerConnector() at runtime, and
reference the ConnectorPlugin interface and registerConnector function so
operators know which shape and API the manifest/codegen must produce.
In `@scripts/__tests__/generate-connector-imports.test.mjs`:
- Around line 40-44: Update the test for validateEntry to better reflect and
assert identifier-syntax failures: rename the spec string from "rejects invalid
export identifier" to something like "rejects invalid export identifier syntax"
and add an explicit assertion that validateEntry({ package: "pkg", export:
"123abc" }, 0) returns the error message containing "must be a valid JavaScript
identifier" (in addition to the existing "not-valid" case) so the intent and a
clear leading-digit identifier failure are both covered.
- Around line 99-128: Add tests that cover multi-entry and generator/validation
edge cases: for renderSource, add a test invoking renderSource with 2+ entries
and assert the output contains both aliases (externalConnector0 and
externalConnector1) and that the imports/entries are separated by a newline; for
runGenerator, add a smoke test using a temp directory that verifies (a) when
manifest is missing an empty file is written, (b) a second run reports wrote:
false (idempotency), and (c) when the manifest contains malformed JSON the
result is ok: false and returns a useful error; and for validateEntry add tests
that ensure it rejects non-boolean overrides and non-string export values.
Reference the existing test suite/helpers renderSource, runGenerator, and
validateEntry when adding these cases.
In `@scripts/generate-connector-imports.mjs`:
- Around line 248-252: The log prints an absolute path on Windows because
OUTPUT_PATH.replace(REPO_ROOT + "/", "") hard-codes "/" as the separator; change
the logging to compute a platform-safe relative path (e.g., using
path.relative(REPO_ROOT, OUTPUT_PATH)) before printing so the console.log in
scripts/generate-connector-imports.mjs (the block checking result.wrote) outputs
a repo-relative path cross-platform; update the reference to use that relative
path variable instead of the string replace.
- Around line 189-200: When manifestPath is missing the function currently
silently writes an empty connector output; update the branch that handles
!existsSync(manifestPath) to emit a single visible warning (e.g., console.warn)
indicating the manifest is absent and external connectors will not be generated
while preserving the current return values ({ ok: true, errors: [], wrote:
...}). Locate the block that calls renderSource([]), checks existing via
existsSync(outputPath) / readFileSync, and writes via writeFileSync, and insert
the console.warn before returning so the behavior of renderSource,
existing/outputPath checks, and writeFileSync remains unchanged except for the
added warning.
🪄 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: 72469a16-8936-4d09-b749-eba7179f4bd5
📒 Files selected for processing (6)
connection/src/connector-registry.tsconnection/src/external-connectors.generated.tsneoboard-connectors.jsonpackage.jsonscripts/__tests__/generate-connector-imports.test.mjsscripts/generate-connector-imports.mjs
…essage CodeRabbit correctly noted the error message hard-coded "built-in" even for external-vs-external collisions. Now checks whether the conflicting connector is neo4j/postgresql (built-in) or a previously-registered external, and adjusts the message accordingly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
3 tasks
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
Mirror the chart plugin pattern for database connectors. External connectors (MongoDB, MySQL, ClickHouse, etc.) can now be installed as npm packages and registered via a
neoboard-connectors.jsonmanifest — no forking required.What's new
neoboard-connectors.json— manifest file (same pattern asneoboard-plugins.json)scripts/generate-connector-imports.mjs— build-time codegen (18 unit tests)connection/src/external-connectors.generated.ts— auto-generated from manifestconnector-registry.ts— loadsEXTERNAL_CONNECTORSafter built-ins with conflict detectionpredev/prebuildscripts updated to run both plugin + connector codegenHow to add an external connector
npm install @myorg/neoboard-mongodb{ "connectors": [ { "package": "@myorg/neoboard-mongodb", "overrides": false } ] }npm run generate:connectors(or it runs automatically on dev/build)The package exports a
ConnectorPluginwithtype,label,category,createModule(), and optionalformFieldsfor auto-generated connection forms.Test plan
Closes #603
🤖 Generated with Claude Code
Summary by CodeRabbit