Clone PR #17925: feat(Todoist Node): Add more resources and operations #4
Conversation
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
Original PR: n8n-io/n8n#17925 Author: mutdmour ## Summary Add more resources and operations to Todoist node https://github.com/user-attachments/assets/4bbee79f-ea0e-4c20-9e34-e00add862cd9 <!-- Describe what the PR does and how to test. Photos and videos are recommended. --> ## Related Linear tickets, Github issues, and Community forum posts <!-- Include links to **Linear ticket** or Github issue or Community forum post. Important in order to close *automatically* and provide context to reviewers. https://linear.app/n8n/issue/ --> <!-- Use "closes #<issue-number>", "fixes #<issue-number>", or "resolves #<issue-number>" to automatically close issues when the PR is merged. --> ## Review / Merge checklist - [ ] PR title and summary are descriptive. ([conventions](../blob/master/.github/pull_request_title_conventions.md)) <!-- **Remember, the title automatically goes into the changelog. Use `(no-changelog)` otherwise.** --> - [ ] [Docs updated](https://github.com/n8n-io/n8n-docs) or follow-up ticket created. - [ ] Tests included. <!-- A bug is not considered fixed, unless a test is added to prevent it from happening again. A feature is not complete without tests. --> - [ ] PR Labeled with `release/backport` (if the PR is an urgent fix that needs to be backported) --- This is a cloned PR for AI code review benchmarking.
🤖 Refacto.AI: PR Summary & Reviewer Guide🎯 1. The Goal (Why was this PR raised?)This PR aims to enhance the Todoist Node by adding more resources and operations. It expands the node's functionality to support additional Todoist API features beyond the existing task operations. 🗺️ 2. The Changes (What was implemented?)This section provides a walkthrough of the changes, connecting them to the PR's goals.
🤔 3. Key Areas for Human ReviewHere are the most important areas to focus your review on, including specific testing instructions. Area of Concern: Type Validation Implementation
Area of Concern: Service Class Architecture
Area of Concern: Operation Handler Implementation
Area of Concern: Node UI Definition
|
|
/do-refacto |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Todoist Node Type Safety Implementation👍 Well Done
📌 Files Processed
📝 Additional Comments
|
![refacto-bot[bot]](https://avatars.githubusercontent.com/in/1142269?s=60&v=4){kind=small}
Comment on lines
159
to
+163
| project_id: projectId, | ||
| priority: options.priority! ? parseInt(options.priority as string, 10) : 1, | ||
| priority: | ||
| typeof options.priority === 'string' | ||
| ? parseInt(options.priority, 10) | ||
| : (options.priority ?? 1), |
There was a problem hiding this comment.
Type Conversion Error in Priority Handling
The priority field handling has inconsistent type conversion. When options.priority is a number, it's used directly, but when it's a string, it's parsed to integer. This creates inconsistent behavior if options.priority is undefined (returns 1) vs null (returns null).
Suggested change
| project_id: projectId, | |
| priority: options.priority! ? parseInt(options.priority as string, 10) : 1, | |
| priority: | |
| typeof options.priority === 'string' | |
| ? parseInt(options.priority, 10) | |
| : (options.priority ?? 1), | |
| priority: options.priority !== undefined | |
| ? (typeof options.priority === 'string' | |
| ? parseInt(options.priority, 10) | |
| : options.priority) | |
| : 1, |
Standards
- Algorithm-Correctness-Type-Conversion
- Logic-Verification-Null-Handling
Comment on lines
+116
to
+118
| const content = ctx.getNodeParameter('content', itemIndex); | ||
| assertIsString('content', content); | ||
|
|
There was a problem hiding this comment.
Missing Error Handling in Content Parameter
The assertIsString function throws errors without specific handling. If content parameter is invalid, the error propagates without proper context, making debugging difficult and reducing system reliability.
Suggested change
| const content = ctx.getNodeParameter('content', itemIndex); | |
| assertIsString('content', content); | |
| try { | |
| const content = ctx.getNodeParameter('content', itemIndex); | |
| assertIsString('content', content); | |
| } catch (error) { | |
| throw new ApplicationError( | |
| 'Invalid content parameter: Must be a string', | |
| { cause: error } | |
| ); | |
| } |
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Reliability-Recoverability
Comment on lines
+173
to
+187
| export function isProjectOperationType(operation: string): operation is ProjectOperationType { | ||
| return PROJECT_OPERATIONS.includes(operation as ProjectOperationType); | ||
| } | ||
|
|
||
| export function isSectionOperationType(operation: string): operation is SectionOperationType { | ||
| return SECTION_OPERATIONS.includes(operation as SectionOperationType); | ||
| } | ||
|
|
||
| export function isCommentOperationType(operation: string): operation is CommentOperationType { | ||
| return COMMENT_OPERATIONS.includes(operation as CommentOperationType); | ||
| } | ||
|
|
||
| export function isLabelOperationType(operation: string): operation is LabelOperationType { | ||
| return LABEL_OPERATIONS.includes(operation as LabelOperationType); | ||
| } |
There was a problem hiding this comment.
Type Guard Duplication
Type guard functions follow identical pattern with duplicated logic. This violates DRY principle and makes maintenance harder when adding new resource types or modifying type guard behavior.
Suggested change
| export function isProjectOperationType(operation: string): operation is ProjectOperationType { | |
| return PROJECT_OPERATIONS.includes(operation as ProjectOperationType); | |
| } | |
| export function isSectionOperationType(operation: string): operation is SectionOperationType { | |
| return SECTION_OPERATIONS.includes(operation as SectionOperationType); | |
| } | |
| export function isCommentOperationType(operation: string): operation is CommentOperationType { | |
| return COMMENT_OPERATIONS.includes(operation as CommentOperationType); | |
| } | |
| export function isLabelOperationType(operation: string): operation is LabelOperationType { | |
| return LABEL_OPERATIONS.includes(operation as LabelOperationType); | |
| } | |
| // Generic type guard factory | |
| function createOperationTypeGuard<T extends string>(operations: readonly T[]) { | |
| return (operation: string): operation is T => { | |
| return operations.includes(operation as T); | |
| }; | |
| } | |
| // Create type guards using the factory | |
| export const isProjectOperationType = createOperationTypeGuard(PROJECT_OPERATIONS); | |
| export const isSectionOperationType = createOperationTypeGuard(SECTION_OPERATIONS); | |
| export const isCommentOperationType = createOperationTypeGuard(COMMENT_OPERATIONS); | |
| export const isLabelOperationType = createOperationTypeGuard(LABEL_OPERATIONS); |
Standards
- Clean-Code-DRY
- Refactoring-Extract-Method
- Design-Pattern-Factory
Comment on lines
+1631
to
+1645
| if (!isProjectOperationType(operation)) { | ||
| throw new NodeOperationError( | ||
| this.getNode(), | ||
| `Invalid operation '${operation}' for project resource`, | ||
| ); | ||
| } | ||
| responseData = await service.executeProject(this, operation, i); | ||
| } else if (resource === 'section') { | ||
| if (!isSectionOperationType(operation)) { | ||
| throw new NodeOperationError( | ||
| this.getNode(), | ||
| `Invalid operation '${operation}' for section resource`, | ||
| ); | ||
| } | ||
| responseData = await service.executeSection(this, operation, i); |
There was a problem hiding this comment.
Type Guard Inconsistency for Task Resource
The task resource doesn't have type guard validation unlike other resources. This inconsistency could allow invalid operations to be passed to executeTask, potentially causing runtime failures.
Suggested change
| if (!isProjectOperationType(operation)) { | |
| throw new NodeOperationError( | |
| this.getNode(), | |
| `Invalid operation '${operation}' for project resource`, | |
| ); | |
| } | |
| responseData = await service.executeProject(this, operation, i); | |
| } else if (resource === 'section') { | |
| if (!isSectionOperationType(operation)) { | |
| throw new NodeOperationError( | |
| this.getNode(), | |
| `Invalid operation '${operation}' for section resource`, | |
| ); | |
| } | |
| responseData = await service.executeSection(this, operation, i); | |
| if (resource === 'task') { | |
| if (!isTaskOperationType(operation)) { | |
| throw new NodeOperationError( | |
| this.getNode(), | |
| `Invalid operation '${operation}' for task resource` | |
| ); | |
| } | |
| responseData = await service.executeTask(this, operation, i); | |
| } else if (resource === 'project') { |
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
Comment on lines
+101
to
+115
| async executeProject( | ||
| ctx: Context, | ||
| operation: ProjectOperationType, | ||
| itemIndex: number, | ||
| ): Promise<TodoistResponse> { | ||
| return await this.projectHandlers[operation].handleOperation(ctx, itemIndex); | ||
| } | ||
|
|
||
| async executeSection( | ||
| ctx: Context, | ||
| operation: SectionOperationType, | ||
| itemIndex: number, | ||
| ): Promise<TodoistResponse> { | ||
| return await this.sectionHandlers[operation].handleOperation(ctx, itemIndex); | ||
| } |
There was a problem hiding this comment.
Service Method Duplication
Multiple execute methods with identical structure for different resource types. This creates code duplication and makes adding new resource types require repetitive boilerplate code.
Suggested change
| async executeProject( | |
| ctx: Context, | |
| operation: ProjectOperationType, | |
| itemIndex: number, | |
| ): Promise<TodoistResponse> { | |
| return await this.projectHandlers[operation].handleOperation(ctx, itemIndex); | |
| } | |
| async executeSection( | |
| ctx: Context, | |
| operation: SectionOperationType, | |
| itemIndex: number, | |
| ): Promise<TodoistResponse> { | |
| return await this.sectionHandlers[operation].handleOperation(ctx, itemIndex); | |
| } | |
| // Generic execute method for any resource type | |
| private async executeOperation<T extends string>( | |
| ctx: Context, | |
| operation: T, | |
| itemIndex: number, | |
| handlers: Record<string, OperationHandler> | |
| ): Promise<TodoistResponse> { | |
| return await handlers[operation].handleOperation(ctx, itemIndex); | |
| } | |
| // Resource-specific methods using the generic execute | |
| async executeTask( | |
| ctx: Context, | |
| operation: TaskOperationType, | |
| itemIndex: number, | |
| ): Promise<TodoistResponse> { | |
| return this.executeOperation(ctx, operation, itemIndex, this.handlers); | |
| } | |
| async executeProject( | |
| ctx: Context, | |
| operation: ProjectOperationType, | |
| itemIndex: number, | |
| ): Promise<TodoistResponse> { | |
| return this.executeOperation(ctx, operation, itemIndex, this.projectHandlers); | |
| } |
Standards
- Clean-Code-DRY
- Refactoring-Extract-Method
- Design-Pattern-Template-Method
Co-authored-by: do-refacto[bot] <198860923+do-refacto[bot]@users.noreply.github.com>
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
Add more resources and operations to Todoist node
Screen.Recording.2025-08-03.at.15.40.57.mov
Related Linear tickets, Github issues, and Community forum posts
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)This is a cloned PR for AI code review benchmarking.
Summary
Related Linear tickets, Github issues, and Community forum posts
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)