feat(Todoist Node): Add more resources and operations #17925
Conversation
- Split monolithic function into focused, single-responsibility functions - Added assertIsValidObject, assertIsRequiredParameter, assertIsResourceLocator - Added assertIsObjectType, assertIsArrayType, assertIsPrimitiveType helpers - Added validateParameterType and validateParameterAgainstTypes coordinators - Reused existing assertIsArray function to reduce code duplication - Maintained exact same functionality while improving maintainability - All existing tests pass 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
@claude review |
|
Claude finished @mutdmour's task —— View job Review Complete ✅
Code Review SummaryThis PR significantly expands the Todoist node by adding support for Projects, Sections, Comments, and Labels resources, along with comprehensive operations for each. The implementation follows good architectural patterns with proper separation of concerns. ✅ StrengthsArchitecture & Design:
Implementation Quality:
Test Coverage:
User Experience:
🔍 Areas for ImprovementMinor Issues:
Suggestions:
🛡️ Security & Best Practices
📊 Test Quality AssessmentThe test suite is comprehensive and well-structured:
🎯 Recommendation✅ APPROVE - This is a high-quality implementation that significantly enhances the Todoist node's functionality. The code follows n8n's patterns well, includes comprehensive tests, and provides a great user experience. The minor issues identified above are not blocking and can be addressed in follow-up improvements. The PR successfully delivers on its promise to "add more resources and operations" with professional-grade implementation quality. |
| const id = ctx.getNodeParameter('taskId', itemIndex); | ||
| assertIsStringOrNumber('taskId', id); |
There was a problem hiding this comment.
maybe these could be merged into its own function, but I have decided this is out of scope here.
| const commandsJson = ctx.getNodeParameter('commands', itemIndex) as string; | ||
| const commandsJson = ctx.getNodeParameter('commands', itemIndex); | ||
| assertIsString('commands', commandsJson); | ||
|
|
||
| const projectId = ctx.getNodeParameter('project', itemIndex, undefined, { |
| project_id?: number | string; | ||
| section?: string; | ||
| content?: string; | ||
| }; | ||
| } | ||
|
|
||
| export const CommandTypes = { | ||
| // Item/Task commands | ||
| ITEM_MOVE: 'item_move', | ||
| ITEM_ADD: 'item_add', | ||
| ITEM_UPDATE: 'item_update', | ||
| ITEM_REORDER: 'item_reorder', |
There was a problem hiding this comment.
Since this has unused command types, so I have decided to complete it for sake completeness. Or should we clean this up?
| description: 'The content of the note', | ||
| }, | ||
| { | ||
| displayName: 'Reminder', |
There was a problem hiding this comment.
this one is weird. adding reminder does not always work for me. But works sometimes. Not sure if it's a limitation on my account or an issue with the API. Will leave it since it kinda works.
I created changes with a "Reminders" resource, but stashed them for now. Can create a separate PR for that once we understand why it does not work consistently.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic analysis
8 issues found across 9 files • Review in cubic
React with 👍 or 👎 to teach cubic. You can also tag @cubic-dev-ai to give feedback, ask questions, or re-run the review.
|
|
||
| // Task operations | ||
| todoistNock.post('/rest/v2/tasks').reply(200, taskData); | ||
| todoistNock.post('/rest/v2/tasks').reply(200, taskData2); |
There was a problem hiding this comment.
The same endpoint is mocked twice in succession; while nock will match them in order, this pattern is brittle and obscures intent. Use .times(2) or chain .reply() with an array to make it explicit that two calls are expected.
Prompt for AI agents
Address the following comment on packages/nodes-base/nodes/Todoist/v2/test/TodoistV2.node.test.ts at line 277:
<comment>The same endpoint is mocked twice in succession; while nock will match them in order, this pattern is brittle and obscures intent. Use `.times(2)` or chain `.reply()` with an array to make it explicit that two calls are expected.</comment>
<file context>
@@ -0,0 +1,347 @@
+import { NodeTestHarness } from '@nodes-testing/node-test-harness';
+import type { WorkflowTestData } from 'n8n-workflow';
+import nock from 'nock';
+
+// Mock data with randomized IDs and generic names
+const projectData = {
+ id: '1234567890',
+ parent_id: null,
+ order: 31,
</file context>
|
|
||
| const successResponse = { success: true }; | ||
|
|
||
| describe('Execute TodoistV2 Node', () => { |
There was a problem hiding this comment.
HTTP mocks created with nock are not cleaned up, which can lead to test leakage and flakiness when more tests are introduced. Add an afterEach that calls nock.cleanAll() (Based on your team's feedback about improving test reliability).
Prompt for AI agents
Address the following comment on packages/nodes-base/nodes/Todoist/v2/test/TodoistV2.node.test.ts at line 249:
<comment>HTTP mocks created with nock are not cleaned up, which can lead to test leakage and flakiness when more tests are introduced. Add an afterEach that calls nock.cleanAll() (Based on your team's feedback about improving test reliability).</comment>
<file context>
@@ -0,0 +1,347 @@
+import { NodeTestHarness } from '@nodes-testing/node-test-harness';
+import type { WorkflowTestData } from 'n8n-workflow';
+import nock from 'nock';
+
+// Mock data with randomized IDs and generic names
+const projectData = {
+ id: '1234567890',
+ parent_id: null,
+ order: 31,
</file context>
| } | ||
|
|
||
| function createElementValidator(elementType: 'string' | 'number' | 'boolean') { | ||
| return (val: unknown): val is string | number | boolean => typeof val === elementType; |
There was a problem hiding this comment.
The type predicate declares that the validator narrows to string | number | boolean independent of the requested elementType, so an array validated for string will still be typed as (string | number | boolean)[]. This weakens compile-time guarantees produced by assertIsArray() and can let type mismatches slip past the compiler. Consider returning a predicate whose narrowing matches the specific elementType that was passed in. (Based on your team's feedback about maintaining accurate TypeScript typings for safer code.)
Prompt for AI agents
Address the following comment on packages/nodes-base/utils/types.ts at line 84:
<comment>The type predicate declares that the validator narrows to `string | number | boolean` independent of the requested `elementType`, so an array validated for `string` will still be typed as `(string | number | boolean)[]`. This weakens compile-time guarantees produced by `assertIsArray()` and can let type mismatches slip past the compiler. Consider returning a predicate whose narrowing matches the specific `elementType` that was passed in. (Based on your team's feedback about maintaining accurate TypeScript typings for safer code.)</comment>
<file context>
@@ -28,67 +38,130 @@ export function assertIsArray<T>(
);
}
+type ParameterType =
+ | 'string'
+ | 'boolean'
+ | 'number'
+ | 'resource-locator'
+ | 'string[]'
</file context>
|
|
||
| // Mock uuid | ||
| jest.mock('uuid', () => ({ | ||
| v4: jest.fn(() => 'mock-uuid-123'), |
There was a problem hiding this comment.
Mocking uuid.v4 with a constant value removes the uniqueness guarantee and can hide bugs related to duplicate IDs in logic that depends on distinct UUIDs (e.g. temp_id vs uuid generation in SyncHandler). Consider returning deterministically unique values for each call instead.
Prompt for AI agents
Address the following comment on packages/nodes-base/nodes/Todoist/v2/test/OperationHandler.test.ts at line 50:
<comment>Mocking uuid.v4 with a constant value removes the uniqueness guarantee and can hide bugs related to duplicate IDs in logic that depends on distinct UUIDs (e.g. temp_id vs uuid generation in SyncHandler). Consider returning deterministically unique values for each call instead.</comment>
<file context>
@@ -0,0 +1,1232 @@
+import { mock } from 'jest-mock-extended';
+import type { IExecuteFunctions, INode } from 'n8n-workflow';
+
+import { todoistApiRequest, todoistSyncRequest } from '../../GenericFunctions';
+import {
+ CreateHandler,
+ CloseHandler,
+ DeleteHandler,
+ GetHandler,
</file context>
| // Mock Context interface | ||
| const createMockContext = (params: Record<string, any> = {}) => | ||
| mock<IExecuteFunctions>({ | ||
| getNodeParameter: jest.fn((key: string) => params[key]), |
There was a problem hiding this comment.
The mocked getNodeParameter ignores the optional default-value and options arguments that the production code frequently relies on (e.g. getNodeParameter('options', idx, {})). Tests will therefore return undefined instead of the supplied default when a key is missing, potentially letting incorrect logic pass unnoticed and reducing test accuracy.
Prompt for AI agents
Address the following comment on packages/nodes-base/nodes/Todoist/v2/test/OperationHandler.test.ts at line 59:
<comment>The mocked getNodeParameter ignores the optional default-value and options arguments that the production code frequently relies on (e.g. getNodeParameter('options', idx, {})). Tests will therefore return undefined instead of the supplied default when a key is missing, potentially letting incorrect logic pass unnoticed and reducing test accuracy.</comment>
<file context>
@@ -0,0 +1,1232 @@
+import { mock } from 'jest-mock-extended';
+import type { IExecuteFunctions, INode } from 'n8n-workflow';
+
+import { todoistApiRequest, todoistSyncRequest } from '../../GenericFunctions';
+import {
+ CreateHandler,
+ CloseHandler,
+ DeleteHandler,
+ GetHandler,
</file context>
| getNodeParameter: jest.fn((key: string) => params[key]), | |
| getNodeParameter: jest.fn((key: string, _idx?: number, defaultValue?: any) => (key in params ? params[key] : defaultValue)), |
| ): Promise<any> { | ||
| const authentication = this.getNodeParameter('authentication', 0, 'oAuth2'); | ||
|
|
||
| const options: IRequestOptions = { | ||
| headers: {}, | ||
| method: 'POST', | ||
| qs, | ||
| uri: 'https://api.todoist.com/sync/v9/sync', | ||
| uri: `https://api.todoist.com/sync/v9${endpoint}`, |
There was a problem hiding this comment.
If a caller supplies an endpoint value that does not begin with a leading slash (e.g. "projects"), the concatenation will generate an invalid URL such as "https://api.todoist.com/sync/v9projects". This brittleness can be avoided by defensively adding the slash when it is missing.
Prompt for AI agents
Address the following comment on packages/nodes-base/nodes/Todoist/GenericFunctions.ts at line 62:
<comment>If a caller supplies an endpoint value that does not begin with a leading slash (e.g. "projects"), the concatenation will generate an invalid URL such as "https://api.todoist.com/sync/v9projects". This brittleness can be avoided by defensively adding the slash when it is missing.</comment>
<file context>
@@ -51,14 +51,15 @@ export async function todoistSyncRequest(
this: Context,
body: any = {},
qs: IDataObject = {},
+ endpoint: string = '/sync',
): Promise<any> {
const authentication = this.getNodeParameter('authentication', 0, 'oAuth2');
const options: IRequestOptions = {
headers: {},
</file context>
| uri: `https://api.todoist.com/sync/v9${endpoint}`, | |
| uri: `https://api.todoist.com/sync/v9${endpoint.startsWith('/') ? endpoint : '/' + endpoint}`, |
| const elementType = | ||
| baseType === 'string' || baseType === 'number' || baseType === 'boolean' ? baseType : 'string'; | ||
|
|
||
| const validator = createElementValidator(elementType as 'string' | 'number' | 'boolean'); |
There was a problem hiding this comment.
Rule violated: Prefer Typeguards over Type casting
Avoid using as for type narrowing; implement a type guard to verify elementType is 'string' | 'number' | 'boolean' before passing it to createElementValidator.
Prompt for AI agents
Address the following comment on packages/nodes-base/utils/types.ts at line 92:
<comment>Avoid using `as` for type narrowing; implement a type guard to verify `elementType` is `'string' | 'number' | 'boolean'` before passing it to `createElementValidator`.</comment>
<file context>
@@ -28,67 +38,130 @@ export function assertIsArray<T>(
);
}
+type ParameterType =
+ | 'string'
+ | 'boolean'
+ | 'number'
+ | 'resource-locator'
+ | 'string[]'
</file context>
| @@ -737,11 +1622,43 @@ export class TodoistV2 implements INodeType { | |||
| const service = new TodoistService(); | |||
| let responseData; | |||
| const resource = this.getNodeParameter('resource', 0); | |||
| const operation = this.getNodeParameter('operation', 0) as OperationType; | |||
| const operation = this.getNodeParameter('operation', 0) as TaskOperationType; | |||
There was a problem hiding this comment.
Rule violated: Prefer Typeguards over Type casting
Direct type casting with as TaskOperationType violates the “Prefer Typeguards over Type casting” rule. Use the generic overload of getNodeParameter instead of a type assertion.
Prompt for AI agents
Address the following comment on packages/nodes-base/nodes/Todoist/v2/TodoistV2.node.ts at line 1625:
<comment>Direct type casting with `as TaskOperationType` violates the “Prefer Typeguards over Type casting” rule. Use the generic overload of `getNodeParameter` instead of a type assertion.</comment>
<file context>
@@ -737,11 +1622,43 @@ export class TodoistV2 implements INodeType {
const service = new TodoistService();
let responseData;
const resource = this.getNodeParameter('resource', 0);
- const operation = this.getNodeParameter('operation', 0) as OperationType;
+ const operation = this.getNodeParameter('operation', 0) as TaskOperationType;
for (let i = 0; i < length; i++) {
try {
</file context>
| const operation = this.getNodeParameter('operation', 0) as TaskOperationType; | |
| const operation = this.getNodeParameter<TaskOperationType>('operation', 0); |
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)