fix: resolve n8n review findings for 0.2.3 (scanner lint + codex node identifier) - #14
Conversation
n8n rejected the 0.2.1 -> 0.2.2 update: 28 scanner lint problems plus one
MEDIUM manual finding on the codex node identifier. The review runs
@n8n/scan-community-package with allowInlineConfig: false, so every
eslint-disable in the repo is ignored.
- Codex "node" is now n8n-nodes-cloudinary.cloudinary (was the legacy
n8n-nodes-base.cloudinary), which is also what n8n's installed-package
detection matches against.
- Errors thrown from execute() and admin/search.ts are wrapped in
NodeApiError/NodeOperationError; metadataToPipeString throws
NodeOperationError and takes the node as a parameter.
- Credential declares the required icon; node and credential use
{light, dark} pairs with the approved 2026 brand logomarks.
- inputs/outputs use NodeConnectionTypes.Main instead of string literals.
- The $json example on Continue From Transformation moved to a hint.
- Curated option lists moved to named consts, preserving order and labels
without suppression comments.
- author gains an email; version bumped to 0.2.3.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
eitanp461
left a comment
eitanp461
left a comment
There was a problem hiding this comment.
Notes on the non-obvious choices, anchored to the lines they explain.
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "node": "n8n-nodes-base.cloudinary", | |||
| "node": "n8n-nodes-cloudinary.cloudinary", | |||
There was a problem hiding this comment.
The MEDIUM review finding. Correcting an earlier version of this comment — I claimed this fix also repairs the duplicate-entry behaviour in the Add-action panel. That was wrong, and the history of this file deserves a straight answer, since it has now flipped n8n-nodes-base → n8n-nodes-cloudinary → back → and now forward again (thanks @sveta-slepner for spotting it).
What this field actually does: nothing at runtime. n8n's codex loader reads only four keys out of .node.json. From packages/core/src/nodes-loader/directory-loader.ts:
private getCodex(filePath: string): CodexData {
const codexFilePath = this.resolvePath(`${filePath}on`); // .js to .json
const { categories, subcategories, resources: { primaryDocumentation, credentialDocumentation }, alias } = module.require(codexFilePath) as Codex;
...
}node, nodeVersion and codexVersion are never destructured — nothing consumes them. So this value cannot break a workflow, a docs link, or node loading, in either direction.
And the dedupe claim was wrong. The vetted-catalog entry that drives the "install this" suggestion comes from n8n's own Strapi API (api.n8n.io/api/community-nodes), not from our repo: isInstalled(nodeType.name) in community-node-types.service.ts:220 uses the catalog's name, which n8n populates on their side. Our codex file has no input to it. I traced this the wrong way round the first time.
So why change it? One reason only: n8n's 2026-08 review flagged the legacy value as a MEDIUM finding, and that review gates the 0.2.3 publish. Secondarily it's what the codex reference describes — <npm package name>.<node name>, i.e. our package.json name plus the descriptor's name: 'cloudinary' (Cloudinary.node.ts:18), with n8n-nodes-base. reserved for built-ins.
Why the earlier revert looked right. In PR #7 the justification was "the codex spec and published community-node practice". Practice genuinely is inconsistent — I checked published packages: n8n-nodes-brightdata uses n8n-nodes-brightdata.BrightData (our new shape), while n8n-nodes-elevenlabs and n8n-nodes-browserless still ship n8n-nodes-base.*, and a few (n8n-nodes-firecrawl, n8n-nodes-apify) never replaced the template's literal n8n-nodes-<name> placeholder. Nothing enforced it, so both values survived review historically. What's new is that n8n now flags it by hand.
Happy to revert if you'd rather not churn the file again — but then we'd need n8n to waive the finding.
| }, | ||
| inputs: ['main'], | ||
| outputs: ['main'], | ||
| inputs: [NodeConnectionTypes.Main], |
There was a problem hiding this comment.
Required by the scanner's @n8n/community-nodes/node-connection-type-literal rule (in its recommended config), which forbids the 'main' string literal and prescribes exactly this form.
Worth naming the trade-off: NodeConnectionTypes only exists in n8n-workflow >= ~1.79, so this raises the package's implicit minimum n8n version, and our peerDependencies is "n8n-workflow": "*". I checked whether keeping the string was an option — it isn't, the gate rejects it. The practical risk is low (the vetted community-node program targets far newer n8n than 1.79), but if we want to be explicit we could declare a floor in a follow-up.
| if (error instanceof NodeApiError) { | ||
| throw new NodeApiError(this.getNode(), error as unknown as JsonObject, { itemIndex: i }); | ||
| } | ||
| throw new NodeOperationError(this.getNode(), error as Error, { itemIndex: i }); |
There was a problem hiding this comment.
Why construct a new error instead of rethrowing the caught one. require-node-api-error flags any throw <catchParam> inside a catch — even when it's instanceof-guarded — so the previous throw error couldn't stay.
Re-wrapping is safe because it's a pass-through: both constructors return an already-wrapped error of their own class untouched (if (errorResponse instanceof NodeApiError) return errorResponse at node-api.error.js:90, same shape at node-operation.error.js:21 in n8n-workflow 2.16.0). So an error a handler already attributed keeps its message, HTTP code and context.
One consequence: in that pass-through branch the { itemIndex: i } option is discarded, since the constructor returns before applying options. Fine for handler-thrown errors (they set their own item context) — see the note on cloudinary.utils.ts for the one place where it means something.
| * calling node. | ||
| */ | ||
| export const metadataToPipeString = (input: IDataObject | string): string => { | ||
| export const metadataToPipeString = (input: IDataObject | string, node: INode): string => { |
There was a problem hiding this comment.
ApplicationError is scanner-flagged, so this now throws NodeOperationError, which needs a node instance — hence the new second parameter. That's the whole reason the four call sites gained ctx.getNode(). User-visible effect: invalid metadata JSON now surfaces as a proper node-attributed error instead of an unattributed internal one.
Known gap, flagged in review: the error carries no itemIndex, and execute() can't add one later (the constructor's pass-through returns before options are applied). So on a multi-item run with bad JSON on item 3, the UI won't point at item 3. Not a regression — the old ApplicationError had neither node nor item attribution — but the fix is small: thread i in from the call sites, which already have it. Happy to do it here or as a follow-up, reviewer's preference.
| } | ||
| throw error; | ||
| // Passes an already-wrapped NodeApiError through untouched. | ||
| throw new NodeApiError(ctx.getNode(), error as JsonObject, { itemIndex: i }); |
There was a problem hiding this comment.
Same lint rule as in execute(): throw error isn't allowed in a catch. helpers.httpRequestWithAuthentication already throws a NodeApiError, so this line is a deliberate no-op pass-through — the original error's message, status code and Cloudinary-specific detail survive untouched. The { itemIndex: i } is inert here for the same reason.
| // Transformations leads the list. Kept as a named const: the action sentence-case | ||
| // rule strips colons (no separator char passes it), but it only inspects inline | ||
| // array literals, so the prefixed labels stand without a suppression comment. | ||
| const TRANSFORM_OPERATION_OPTIONS: INodePropertyOptions[] = [ |
There was a problem hiding this comment.
The Compose:/Image:/Video: prefixes are unchanged — both name and action are byte-identical to master. Only their container moved.
I first tried swapping the colon for another separator. Nothing works: node-param-operation-option-action-miscased runs the label through the sentence-case npm package, which strips all punctuation (colon, dash, pipe, slash) and lowercases every word after the first — so no separator character and no capitalised word can pass while the array is inline.
The escape hatch is that every option-inspecting rule (unsorted-items, action-miscased) only analyses inline array literals and bails on options: SOME_CONST (hasPropertyPointingToIdentifier in the rule's getter). tsc keeps the identifier reference in the emitted JS, so this survives both scanner legs — source tree and packed tarball. The same trick keeps RESOURCE_OPTIONS in its curated order (Upload first, deprecated legacy last) instead of alphabetised.
| // rule only inspects descriptions and would rewrite the lowercase `$json` variable | ||
| // to `$JSON`, which is undefined in n8n. Same approach as the base Webhook and | ||
| // Elasticsearch nodes. | ||
| hint: 'e.g. {{ $json.transformation }} from a previous Transform step', |
There was a problem hiding this comment.
Why the example moved to hint. node-param-description-miscased-json matches /\b(j|J)son\b/ — which catches the $json variable — and autofixes it to $JSON, which is undefined in n8n. It fires only on the description field.
I checked how n8n's own base nodes handle this. Three approaches exist: an inline eslint-disable (Webhook's "Only Run If" — useless to us, the review scanner ignores disables), the expression in placeholder (Webhook, Elasticsearch, Firestore), or the example in hint (Elasticsearch and Firestore: "Build the array inside the expression, for example {{ [$json.name, 30] }}..."). We already use placeholder for the literal transformation-string format, so hint was the free slot. There's no miscased-json rule for hint — only hint-untrimmed and hint-url-missing-protocol.
placeholder and description on this field are unchanged.
| // curated, purpose-driven order without a suppression comment. | ||
|
|
||
| // Ordered by relevance (autoplay/sound/loop first), not alphabetically. | ||
| const PLAYBACK_OPTIONS: INodeProperties[] = [ |
There was a problem hiding this comment.
Pure structural move — zero behaviour change. The four eslint-disable-next-line n8n-nodes-base/node-param-collection-type-unsorted-items comments are gone (the scanner ignores them anyway), and the four inline collection arrays are lifted verbatim into PLAYBACK_OPTIONS, LAYOUT_OPTIONS, AI_CONTENT_OPTIONS and APPEARANCE_OPTIONS, which the v2 collections now reference.
I diffed removed vs added lines sorted: no field, label, default or displayOptions differs. The v1 flat player* params and the Player Features block are untouched, so typeVersion 1 workflows are unaffected. The large diff line count is almost entirely re-indentation.
| export class CloudinaryApi implements ICredentialType { | ||
| name = 'cloudinaryApi'; | ||
| displayName = 'Cloudinary API'; | ||
| icon: Icon = { light: 'file:cloudinary.svg', dark: 'file:cloudinary.dark.svg' }; |
There was a problem hiding this comment.
The scanner requires credential classes to declare an icon. Note the duplicated SVG files this needs: n8n resolves file: icon paths relative to the declaring class file, so the node resolves into dist/nodes/Cloudinary/ and the credential into dist/credentials/. Per-directory copies are the community-node convention; there's no shared-asset path. All four files are in the packed tarball.
2 participants
n8n rejected the 0.2.1 → 0.2.2 update. Their review runs
@n8n/scan-community-package@beta, which lints withallowInlineConfig: false— every// eslint-disablein the repo is ignored — plus one manual MEDIUM finding. This PR fixes all 28 lint problems and the MEDIUM finding, and bumps the version to 0.2.3 (publishing to npm auto-triggers re-review).Inline comments on the diff explain the non-obvious choices.
Codex
nodeidentifier (MEDIUM finding)Cloudinary.node.json"node"changed fromn8n-nodes-base.cloudinaryton8n-nodes-cloudinary.cloudinary.It does nothing at runtime. n8n's codex loader (
packages/core/src/nodes-loader/directory-loader.ts,getCodex()) destructures onlycategories,subcategories,resourcesandaliasout of.node.json—node,nodeVersionandcodexVersionare never read. So this value can't break a workflow, a docs link, or node loading, in either direction. Saved workflows store the type n8n core registers asknown.nodes[\${packageName}.${type}`], which has always beenn8n-nodes-cloudinary.cloudinary`.Why change it then? n8n's 2026-08 review flagged the legacy value as a MEDIUM finding, and that review gates the publish. It's also the shape the codex reference describes —
<npm package name>.<node name>, withn8n-nodes-base.reserved for built-ins.Note on the file's history: this value has flipped base → cloudinary → base → cloudinary. The June revert (PR #7) cited "the codex spec and published community-node practice", and practice genuinely is inconsistent —
n8n-nodes-brightdatauses the package-name shape,n8n-nodes-elevenlabsandn8n-nodes-browserlessstill shipn8n-nodes-base.*, and some published packages never replaced the template'sn8n-nodes-<name>placeholder. Nothing enforced it before; what's new is n8n flagging it by hand. See the inline comment for the full trace.Scanner lint fixes
execute()andadmin/search.tswrap thrown errors inNodeApiError/NodeOperationError;metadataToPipeStringthrowsNodeOperationErrorinstead ofApplicationErrorand now takes the node as a parameter (that's the one-line ripple across the four call sites).icon; node and credential use{light, dark}pairs. Icons replaced with the approved logomarks from the 2026 Brand Assets collection: navy circle for light theme, white circle for dark (2.3KB each, down from 58KB).inputs/outputs—NodeConnectionTypes.Maininstead of the'main'string literal, per the scanner's@n8n/community-nodes/node-connection-type-literalrule.$jsonexample — moved fromdescriptiontohint(see inline comment).package.json— author now includes an email (info@cloudinary.com, matching the officialcloudinarynpm package).Curated option ordering preserved
The scanner's option-inspecting rules (alphabetize, action sentence-case) only inspect inline array literals; an
options:pointing at a named const is skipped. So the curated lists — resource dropdown with Upload first, theCompose:/Image:/Video:prefixed Transform operations, the Fit gradient, the four Video Player collections — are extracted to consts, keeping their order and labels with no suppression comments. Every name, value, default anddisplayOptionsis byte-identical to before; the two Fit dropdowns now derive from one shared const so they can't drift.Verification
analyzePackageover both the source tree and the packed 0.2.3 tarball): both legs pass.npm run build,npm run lint, prepublish lint config: clean.npm test: 242/242.🤖 Generated with Claude Code