-
Notifications
You must be signed in to change notification settings - Fork 1
feat: n8n node with v1/search, contents, and express agent #27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These don't actually test any integration. Also we have plenty test against the api as MCP ships default fetch handlers for these endpoint I already reuse in the ai-sdk-plugin. Delete this unless you're specially testing a fetch function of your own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ty! Removed. This appears to be overkill anyway when compared to competitor nodes https://github.com/tavily-ai/tavily-n8n-node/tree/main
| @@ -0,0 +1,84 @@ | |||
| { | |||
| "name": "n8n-nodes-youdotcom", | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename this we have a NPM org @youdotcom-oss see other package.json files for pattern unless n8n wants this a specific way. It should be something like @youdotcom-oss/n8n-nodes
You should also rename the directory to we don't do youdotcom suffixes in this project. please follow the pattern of the other packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EdwardIrby it unfortunately wouldn't pass n8ns validation. It looks like they require package names to follow the convention "n8n-nodes-[PACKAGE-NAME]" or "@[AUTHOR]/n8n-nodes-[PACKAGE-NAME]". I'm curious what your preferences would be here. I'm happy to create a separate repo for the n8n node if the convention isn't flexible.
- Refactor YouDotCom node into proper class with static private methods - Add Zod schemas for runtime validation of inputs and API responses - Remove 7 unnecessary type casts using TypeScript inference - Enhance error handling with detailed Zod validation messages - Remove redundant integration tests (duplicates MCP package tests) - Colocate test file with implementation (node.spec.ts -> YouDotCom.node.spec.ts) - Add zod@4.3.6 dependency All 43 unit tests passing, all quality checks passing.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
- Add .describe() to all Zod schema fields for better error messages - Strengthen schemas with .enum() for freshness, safesearch, livecrawl - Add domain validation for site field with .refine() - Improve Express response schema with stricter validation (non-empty strings, URL validation) - Add SearchResponseSchema and ContentsResponseSchema for API response validation - Improve error message formatting (multiline with numbered list) - Update README.md to remove references to non-existent test scripts All changes address code review suggestions from PR #28: - Stricter input validation catches invalid values before API calls - Response validation detects API contract changes early - Better error messages improve debugging experience - Accurate documentation matches available commands Tests: ✅ All 43 tests passing Quality: ✅ Biome, TypeScript, package format checks pass
|
Claude finished @tyler5673's task in 2m 26s —— View job Code Review Complete ✅SummaryThis PR demonstrates excellent code quality with thorough testing, proper security practices, and clean implementation. All previous feedback has been addressed. The code is production-ready with just two minor documentation fixes needed before merge. 🎯 What's ExcellentCode Quality ✅
Security ✅
Implementation ✅
Documentation ✅
🔧 Issues to Fix (2 minor)1. README Package Name (Lines 10, 17)Issue: Installation instructions use unscoped name Fix: Update both installation commands to use the scoped package name: npm install @youdotcom-oss/n8n-nodes-youdotcom2. User-Agent String (YouDotCom.node.ts:16)Issue: User-Agent uses unscoped name Fix: const USER_AGENT = `@youdotcom-oss/n8n-nodes-youdotcom/${PACKAGE_VERSION} (https://github.com/youdotcom-oss/dx-toolkit)`;📋 Package Naming Discussionn8n Naming Requirements ✅Tyler correctly identified that n8n requires the
Current naming: This correctly follows the scoped format with proper You.com branding while meeting n8n's requirements. Monorepo Convention ExceptionThe monorepo convention (AGENTS.md:90-104) expects packages like Recommendation: Document this exception in AGENTS.md naming section: **Exception:** n8n packages use `@youdotcom-oss/n8n-nodes-[PACKAGE]` naming because n8n
community nodes require the `n8n-nodes-*` prefix for package discovery per n8n's ESLint
validation rules.✅ Previous Feedback AddressedIntegration Tests - RESOLVED ✅EdwardIrby's concern: "These don't actually test any integration... Delete this unless you're specially testing a fetch function of your own." Status: ✅ RESOLVED - Integration tests file completely removed (commit b2c868b). Current test coverage: 43 unit tests validating node configuration, operations, parameters, and credentials. This is appropriate for an n8n node wrapper. 💡 Additional Observations (Non-Blocking)
🎯 Before Merge Checklist
Final VerdictCode Quality: ✅ Excellent - Production-ready implementation Security: ✅ Strong - No vulnerabilities, proper key protection, good error handling Testing: ✅ Comprehensive - 43 tests with 120 assertions covering all features Issues: 🔧 2 minor documentation fixes - Package name consistency in README and User-Agent Recommendation: APPROVE with minor fixes ✅ This is outstanding work with clean implementation, thorough testing, and excellent attention to security. The two naming consistency issues are trivial and can be fixed quickly. Great job, @tyler5673! 🚀 |
| Follow the [installation guide](https://docs.n8n.io/integrations/community-nodes/installation/) in the n8n community nodes documentation. | ||
|
|
||
| ```bash | ||
| npm install @youdotcom-oss/n8n-nodes-youdotcom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Package Name Should Be Scoped
Issue: The installation command uses the unscoped package name, but package.json specifies @youdotcom-oss/n8n-nodes-youdotcom (scoped).
Fix:
npm install @youdotcom-oss/n8n-nodes-youdotcomAlso needs to be updated on line 17 in the UI installation instructions.
| const PACKAGE_VERSION = '0.1.0'; | ||
|
|
||
| /** User-Agent string for API requests */ | ||
| const USER_AGENT = `n8n-nodes-youdotcom/${PACKAGE_VERSION} (https://github.com/youdotcom-oss/dx-toolkit)`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User-Agent Should Match Scoped Package Name
Current: Uses n8n-nodes-youdotcom (unscoped)
Suggested: Match the scoped package name from package.json:
const USER_AGENT = `@youdotcom-oss/n8n-nodes-youdotcom/${PACKAGE_VERSION} (https://github.com/youdotcom-oss/dx-toolkit)`;This provides better identification for API usage tracking and matches the published npm package name.
| @@ -0,0 +1,88 @@ | |||
| { | |||
| "name": "@youdotcom-oss/n8n-nodes-youdotcom", | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Package Naming Verified
Tyler correctly confirmed that n8n requires the n8n-nodes-* prefix based on n8n's ESLint rule. The allowed formats are:
n8n-nodes-[PACKAGE-NAME](unscoped)@[AUTHOR]/n8n-nodes-[PACKAGE-NAME](scoped) ✅
Current naming: @youdotcom-oss/n8n-nodes-youdotcom ✅
This correctly follows the scoped format. The -youdotcom suffix distinguishes it from other packages in the monorepo while meeting n8n's requirements.
Note: This is an exception to the monorepo convention (similar to how n8n requires classes instead of arrow functions). Consider documenting this in AGENTS.md.
| displayName: 'API Key', | ||
| name: 'apiKey', | ||
| type: 'string', | ||
| typeOptions: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Security: API Key Properly Protected
Good security practices:
- ✅
typeOptions: { password: true }masks the API key in n8n's UI - ✅ Credential test validates the key without exposing it (lines 37-47)
- ✅ Uses n8n's built-in credential management
- ✅ No hardcoded secrets
Authentication:
- Search/Contents APIs:
X-API-Keyheader (line 32) - Express API:
Bearertoken (YouDotCom.node.ts:605)
Both methods are properly implemented. No changes needed.
| returnData.push(...executionData); | ||
| } | ||
| } catch (error) { | ||
| if (this.continueOnFail()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Error Handling: Well Implemented
Good practices:
- ✅ Respects
continueOnFail()setting for batch processing - ✅ Provides detailed error context with
itemIndex - ✅ Uses proper n8n error types (
NodeApiError) - ✅ Graceful degradation - errors don't stop the entire workflow
This follows n8n best practices perfectly.
| }; | ||
|
|
||
| // Parse comma-separated URLs and trim whitespace | ||
| const urls = urlsString |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Input Sanitization: Good Implementation
Secure URL parsing:
- ✅ Safely splits comma-separated input
- ✅ Trims whitespace from each URL
- ✅ Filters empty strings
- ✅ Validates at least one URL exists (lines 550-552)
No injection risks - URLs are passed to the API, not executed. Clear error message if validation fails.
| agent?: string; | ||
| }; | ||
|
|
||
| // Transform response to a more user-friendly format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Response Transformation: User-Friendly Design
Excellent UX implementation:
- ✅ Transforms raw API output to intuitive format
- ✅ Extracts
answerfrommessage.answertype - ✅ Extracts
searchResultsfromweb_search.resultstype - ✅ Clear, predictable output structure
Result:
{
agent: "express",
answer: "AI-generated response...",
searchResults: [{ url, title, snippet }, ...]
}This makes the response much easier to work with in n8n workflows compared to the raw API format.
|
Claude finished @tyler5673's task in 2m 43s —— View job Code Review Complete ✅SummaryThis PR demonstrates excellent code quality with thorough testing, proper security practices, and clean implementation. All previous feedback has been addressed (integration tests removed per EdwardIrby's request). The code is production-ready with just one minor suggestion (User-Agent enhancement). ✅ What's ExcellentCode Quality
Security
Implementation
Documentation
💡 Minor Suggestion (Non-Blocking)User-Agent Enhancement (YouDotCom.node.ts:24): Consider including the npm org scope for better API usage tracking: const USER_AGENT = `@youdotcom-oss/n8n-nodes-youdotcom/${PACKAGE_VERSION} (https://github.com/youdotcom-oss/dx-toolkit)`;📋 Package Naming Discussionn8n Naming Requirements ✅Tyler correctly identified that n8n requires the
Current naming: This correctly follows the scoped format with proper You.com branding while meeting n8n's requirements. Monorepo Convention ExceptionThe monorepo convention (AGENTS.md:90-104) expects packages like Recommendation: Document this exception in AGENTS.md naming section: **Exception**: n8n packages use `@youdotcom-oss/n8n-nodes-[PACKAGE]` naming because n8n
community nodes require the `n8n-nodes-*` prefix for package discovery per n8n's ESLint
validation rules.✅ Previous Feedback AddressedIntegration Tests - RESOLVED ✅EdwardIrby's concern: "These don't actually test any integration... Delete this unless you're specially testing a fetch function of your own." Status: ✅ RESOLVED - Integration tests file completely removed (commit b2c868b). Current test coverage: 43 unit tests validating node configuration, operations, parameters, and credentials. This is appropriate and comprehensive for an n8n node wrapper. 📝 Additional Observations
🎯 Final VerdictCode Quality: ✅ Excellent - Production-ready implementation Security: ✅ Strong - No vulnerabilities, proper key protection, good error handling Testing: ✅ Comprehensive - 43 tests with 120 assertions covering all features Issues: 💡 One optional enhancement - User-Agent string could include package scope Recommendation: APPROVE ✅ This is outstanding work with clean implementation, thorough testing, and excellent attention to security and user experience. The n8n community will benefit from having a high-quality You.com integration. Great job, @tyler5673! 🚀 Before publishing:
|
| Follow the [installation guide](https://docs.n8n.io/integrations/community-nodes/installation/) in the n8n community nodes documentation. | ||
|
|
||
| ```bash | ||
| npm install @youdotcom-oss/n8n-nodes-youdotcom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧 Package Name Mismatch
Issue: Installation command shows @youdotcom-oss/n8n-nodes-youdotcom which is correct and matches package.json.
Previous versions of my review incorrectly flagged this as an issue. The README is actually correct - it uses the scoped package name that matches package.json:2.
No changes needed here. ✅
| const PACKAGE_VERSION = '0.1.0'; | ||
|
|
||
| /** User-Agent string for API requests */ | ||
| const USER_AGENT = `n8n-nodes-youdotcom/${PACKAGE_VERSION} (https://github.com/youdotcom-oss/dx-toolkit)`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Consider Including Package Scope in User-Agent
Current: n8n-nodes-youdotcom/${PACKAGE_VERSION}
Suggestion: Consider including the npm org scope for better identification:
const USER_AGENT = `@youdotcom-oss/n8n-nodes-youdotcom/${PACKAGE_VERSION} (https://github.com/youdotcom-oss/dx-toolkit)`;Benefits:
- Clearer identification in API logs
- Matches published npm package name
- Includes GitHub repo for support inquiries
This is a minor enhancement, not blocking.
| displayName: 'API Key', | ||
| name: 'apiKey', | ||
| type: 'string', | ||
| typeOptions: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Security: API Key Properly Protected
Excellent security implementation:
- ✅
typeOptions: { password: true }masks the API key in n8n's UI - ✅ Credential test validates the key without exposing it (lines 37-47)
- ✅ Uses n8n's built-in credential management
- ✅ No hardcoded secrets
Authentication methods:
- Search/Contents APIs:
X-API-Keyheader (line 32) - Express API:
Bearertoken (YouDotCom.node.ts:648)
Both authentication methods are properly implemented. No changes needed.
| }); | ||
| returnData.push(...executionData); | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Error Handling: Excellent Implementation
Good practices:
- ✅ Handles Zod validation errors with detailed messages (lines 478-498)
- ✅ Respects
continueOnFail()setting for batch processing - ✅ Provides detailed error context with
itemIndex - ✅ Uses proper n8n error types (
NodeApiError) - ✅ Graceful degradation - errors don't stop the entire workflow
Error flow:
- Catches Zod validation errors separately with formatted messages
- Catches other errors generically
- If
continueOnFail()is true: Returns error object, continues processing - If
continueOnFail()is false: ThrowsNodeApiErrorwith item context
This follows n8n best practices perfectly. No changes needed.
| const options = ContentsOptionsSchema.parse(rawOptions); | ||
|
|
||
| // Parse comma-separated URLs and trim whitespace | ||
| const urls = urlsString |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Input Sanitization: Good Implementation
Secure URL parsing:
- ✅ Safely splits comma-separated input
- ✅ Trims whitespace from each URL
- ✅ Filters empty strings
- ✅ Validates at least one URL exists (lines 582-584)
Security considerations:
- No injection risks (URLs are passed to API, not executed)
- Proper validation before API call
- Clear error message if validation fails
No changes needed.
| // Validate API response with Zod schema | ||
| const response = ExpressResponseSchema.parse(rawResponse); | ||
|
|
||
| // Transform response to a more user-friendly format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Response Transformation: User-Friendly Design
Excellent UX implementation:
- ✅ Transforms raw API output to intuitive format
- ✅ Extracts
answerfrommessage.answertype (line 668-669) - ✅ Extracts
searchResultsfromweb_search.resultstype (line 670-671) - ✅ Clear, predictable output structure
Result format:
{
agent: "express",
answer: "AI-generated response...",
searchResults: [{ url, title, snippet }, ...]
}This makes the response much easier to work with in n8n workflows compared to the raw API format. Well done!
| @@ -0,0 +1,91 @@ | |||
| { | |||
| "name": "@youdotcom-oss/n8n-nodes-youdotcom", | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Package Naming: Correct Implementation
n8n requires the n8n-nodes-* prefix per their ESLint validation rule.
Allowed formats:
- Unscoped:
n8n-nodes-[PACKAGE-NAME] - Scoped:
@[AUTHOR]/n8n-nodes-[PACKAGE-NAME]✅ (This package uses this format)
Current naming: @youdotcom-oss/n8n-nodes-youdotcom ✅
This correctly follows the scoped format. The -youdotcom suffix distinguishes it from other You.com packages in the monorepo.
Note: This is an exception to the monorepo convention (similar to how n8n requires classes instead of arrow functions). Consider documenting this in AGENTS.md as suggested in previous reviews.
|
Claude finished @tyler5673's task in 3m 0s —— View job Code Review Complete ✅SummaryThis PR adds a high-quality n8n community node for You.com APIs with excellent implementation. The code is production-ready with comprehensive testing, proper security practices, outstanding error handling, and great attention to user experience. All previous feedback has been properly addressed. ✅ What's ExcellentCode Quality
Security
Implementation
Documentation
📋 Previous Feedback: All Resolved ✅Integration Tests - RESOLVED ✅EdwardIrby's concern: "These don't actually test any integration... Delete this unless you're specially testing a fetch function of your own." Status: ✅ RESOLVED - Integration tests file completely removed (commit b2c868b). Current coverage: 43 unit tests validating node configuration, operations, parameters, and credentials. This is appropriate and comprehensive for an n8n node wrapper. The underlying API functionality is tested in the Package Naming - RESOLVED ✅EdwardIrby's concern: "Rename this we have a NPM org @youdotcom-oss... unless n8n wants this a specific way." Status: ✅ RESOLVED - Tyler correctly identified that n8n requires the Current implementation: This is an exception to the monorepo convention (similar to how n8n requires classes instead of arrow functions), and is properly documented in the code with TSDoc comments. 💡 Additional Observations (Non-Blocking)
🎯 Final VerdictCode Quality: ✅ Excellent - Production-ready implementation Security: ✅ Strong - No vulnerabilities, proper key protection, excellent error handling Testing: ✅ Comprehensive - 43 tests with 120 assertions covering all features Standards Compliance: ✅ Good - Follows project patterns with documented exceptions for n8n requirements Recommendation: APPROVE ✅ This is outstanding work with clean implementation, thorough testing, and excellent attention to security and user experience. The n8n community will benefit from having a high-quality You.com integration with support for Search, Contents extraction, and AI-powered Express agent. Great job, @tyler5673! 🚀 Before publishing (non-blocking):
|
| @@ -0,0 +1,91 @@ | |||
| { | |||
| "name": "@youdotcom-oss/n8n-nodes-youdotcom", | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Package Naming: Correctly Implements n8n Requirements
Excellent work resolving the naming discussion! The package correctly follows n8n's validation rules:
n8n's requirement (source):
- Scoped format:
@[AUTHOR]/n8n-nodes-[PACKAGE-NAME]✅
Current implementation:
- Package:
@youdotcom-oss/n8n-nodes-youdotcom✅ - Directory:
packages/n8n-nodes-youdotcom/✅ - Maintains You.com branding while meeting n8n framework requirements
Monorepo alignment: While this differs from the typical pattern (@youdotcom-oss/mcp → packages/mcp/), it's justified because n8n's ESLint rule validates package names and requires the n8n-nodes-* prefix.
Note: This is an exception to the monorepo convention, similar to how n8n requires classes instead of arrow functions. This is properly documented in the code with TSDoc comments.
| displayName: 'API Key', | ||
| name: 'apiKey', | ||
| type: 'string', | ||
| typeOptions: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Security: API Key Properly Protected
Excellent security implementation:
- ✅
typeOptions: { password: true }masks the API key in n8n's UI (line 19-20) - ✅ Credential test validates the key without exposing it (lines 37-47)
- ✅ Uses n8n's built-in credential management framework
- ✅ No hardcoded secrets anywhere in the codebase
Authentication methods:
- Search/Contents APIs:
X-API-Keyheader (line 32) - Express API:
Bearertoken (YouDotCom.node.ts:648)
Both authentication methods are properly secured with different approaches based on API requirements. No changes needed.
| }); | ||
| returnData.push(...executionData); | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Error Handling: Excellent Implementation
Outstanding error handling implementation:
- ✅ Separate handling for Zod validation errors with detailed messages (lines 478-498)
- ✅ Respects
continueOnFail()setting for batch processing workflows - ✅ Provides detailed error context with
itemIndexfor debugging - ✅ Uses proper n8n error types (
NodeApiError,NodeOperationError) - ✅ Graceful degradation - errors don't stop the entire workflow
Error flow:
- Catches Zod validation errors separately with formatted, user-friendly messages
- Catches other errors generically
- If
continueOnFail()is true: Returns error object as data, continues processing - If
continueOnFail()is false: ThrowsNodeApiErrorwith item context
This follows n8n best practices perfectly. Well done!
| const options = ContentsOptionsSchema.parse(rawOptions); | ||
|
|
||
| // Parse comma-separated URLs and trim whitespace | ||
| const urls = urlsString |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Input Sanitization: Good Implementation
Secure URL parsing (lines 577-584):
- ✅ Safely splits comma-separated input
- ✅ Trims whitespace from each URL
- ✅ Filters empty strings
- ✅ Validates at least one URL exists before API call
Security considerations:
- No injection risks (URLs are passed to API, not executed locally)
- Proper validation with clear error message if validation fails
- Edge cases handled (empty strings, extra whitespace)
No changes needed.
| // Validate API response with Zod schema | ||
| const response = ExpressResponseSchema.parse(rawResponse); | ||
|
|
||
| // Transform response to a more user-friendly format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Response Transformation: User-Friendly Design
Excellent UX implementation (lines 660-676):
- ✅ Transforms raw API output to intuitive, flat structure
- ✅ Extracts
answerfrommessage.answertype (lines 668-669) - ✅ Extracts
searchResultsfromweb_search.resultstype (lines 670-671) - ✅ Clear, predictable output format
Result format:
{
agent: "express",
answer: "AI-generated response with citations...",
searchResults: [{ url, title, snippet }, ...]
}This makes the Express API response much easier to work with in n8n workflows compared to the raw API format. Users can directly access the answer and search results without complex data manipulation. Great attention to user experience!
| snippets: z.array(z.string()).optional().describe('Content snippets'), | ||
| page_age: z.string().optional().describe('Publication timestamp'), | ||
| }) | ||
| .passthrough(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Schema Design: Excellent Use of passthrough()
Outstanding schema design (lines 39-76):
- ✅ Validates required fields (
url,title,description) - ✅ Uses
.passthrough()to preserve additional API fields - ✅ Clear JSDoc comments explaining the strategy (lines 32-38)
Why this is excellent:
- Type safety: Required fields are validated
- Future-proof: New API fields automatically pass through
- No data loss: Users get all API data (snippets, page_age, etc.)
- Flexibility: API can add fields without breaking the node
This pattern (validate core fields + passthrough) is the right approach for API wrappers. It balances type safety with flexibility. The same pattern is consistently applied to all response schemas (Search, Contents, Express). Well done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EdwardIrby lmk if you have strong feelings against using passthrough. The schema was stripping out fields it didn't explicitly know about, which meant the response was coming back empty for search. Using passthrough seemed like a good choice for future proofing, and since the n8n node is a thin API wrapper where users expect access to the full API response, this sounded like the right choice to me vs a more explicit schema.
|
Claude finished @tyler5673's task in 1m 44s —— View job Code Review Complete ✅SummaryThis PR adds a high-quality n8n community node for You.com APIs with excellent implementation. The code is production-ready with comprehensive testing, proper security practices, outstanding error handling, and great attention to user experience. All previous review feedback has been properly addressed. ✅ What's ExcellentCode Quality
Security
Implementation
Documentation
📋 Previous Feedback: All Resolved ✅Integration Tests - RESOLVED ✅EdwardIrby's concern: "These don't actually test any integration... Delete this unless you're specially testing a fetch function of your own." Status: ✅ RESOLVED - Integration tests file completely removed (commit b2c868b). Current coverage: 43 unit tests validating node configuration, operations, parameters, and credentials. This is appropriate and comprehensive for an n8n node wrapper. The underlying API functionality is tested in the Package Naming - RESOLVED ✅EdwardIrby's concern: "Rename this we have a NPM org @youdotcom-oss... unless n8n wants this a specific way." Status: ✅ RESOLVED - Tyler correctly identified that n8n requires the Current implementation: This is an exception to the monorepo convention (similar to how n8n requires classes instead of arrow functions), and is properly documented in the code with TSDoc comments. 💡 Schema Design HighlightThe Zod schema design is particularly well done (YouDotCom.schemas.ts:39-76): const WebResultSchema = z.object({
url: z.string(),
title: z.string(),
description: z.string(),
snippets: z.array(z.string()).optional(),
page_age: z.string().optional(),
}).passthrough(); // ← Preserves all API fieldsWhy this is excellent:
This pattern (validate core fields + passthrough) is consistently applied to all response schemas (Search, Contents, Express). Perfect for API wrappers! 📝 Additional Observations (Non-Blocking)
🎯 Final VerdictCode Quality: ✅ Excellent - Production-ready implementation Security: ✅ Strong - No vulnerabilities, proper key protection, excellent error handling Testing: ✅ Comprehensive - 43 tests with 120 assertions covering all features Standards Compliance: ✅ Good - Follows project patterns with documented exceptions for n8n requirements Recommendation: APPROVE ✅ This is outstanding work with clean implementation, thorough testing, and excellent attention to security and user experience. The n8n community will benefit from having a high-quality You.com integration with support for Search, Contents extraction, and AI-powered Express agent. Great job, @tyler5673! 🚀 Before publishing (non-blocking):
|
This is an n8n node that supports v1/search, contents, and express agent usage.
This PR has been well tested in a local n8n environment, and has integration and unit tests included
n8n_integration.mov