Skip to content

Conversation

@gandalf-repo
Copy link

Cloned Pull Request

This PR is a clone of the original PR: n8n-io/n8n#17925

Original Author: mutdmour
Original PR Number: #17925
Original Repository: n8n-io/n8n


Original Description:

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

  • PR title and summary are descriptive. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.
  • PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

Files Changed:

  • modified: packages/nodes-base/nodes/Todoist/GenericFunctions.ts
  • modified: packages/nodes-base/nodes/Todoist/v2/OperationHandler.ts
  • modified: packages/nodes-base/nodes/Todoist/v2/Service.ts
  • modified: packages/nodes-base/nodes/Todoist/v2/TodoistV2.node.ts
  • added: packages/nodes-base/nodes/Todoist/v2/test/OperationHandler.test.ts
  • added: packages/nodes-base/nodes/Todoist/v2/test/TodoistV2.node.test.ts
  • added: packages/nodes-base/nodes/Todoist/v2/test/workflow.json
  • added: packages/nodes-base/utils/__tests__/types.test.ts
  • modified: packages/nodes-base/utils/types.ts

This PR was automatically cloned for AI code review benchmarking purposes.

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.
@gandalf-repo
Copy link
Author

@CodeAnt-AI: review

Please review this code for potential issues, security vulnerabilities, and improvements.

Review triggered at: 2025-08-05T10:44:21.112Z

@codeant-ai
Copy link

codeant-ai bot commented Aug 5, 2025

Question: review

please review this code for potential issues, security vulnerabilities, and improvements.

review triggered at: 2025-08-05t10:44:21.112z

Answer:
Here’s a focused review of the changes in this PR, broken down by category. I’ve tried to call out both small improvements and more strategic refactors, as well as anything that might have security or reliability implications.

  1. General architecture & DRY
    • You have 5 full sets of handlers (Task, Project, Section, Comment, Label) each with very similar CRUD structure. You might consider factoring out a small generic “CrudHandler” base class or helper that takes the HTTP method + path template + list of required/optional params, and then have each concrete handler simply declare its config.
    • All of the calls to ctx.getNodeParameter(...) + assertIsStringOrNumber + building the body object follow the same pattern. A small helper like:

    function getValidatedParam<T extends 'string'|'number'>(ctx, name, itemIndex, type: T, options?: ExtractOptions): string | number {  }

    could cut down on boilerplate.

  2. Input validation & error handling
    • You’ve introduced assertIsString, assertIsNumber, and assertIsNodeParameters—nice! However, when they throw, they throw a generic ApplicationError. For consistency with other n8n nodes you probably want to throw NodeOperationError (so the UI surfaces them as “OperationError” instead of a 500).
    • In the sync handler you do:

    const commands: Command[] = jsonParse(commandsJson);

    That will throw an ApplicationError on invalid JSON, which you then let bubble up. It might befriend to catch it and re-wrap as NodeOperationError so the user sees “Invalid JSON for commands” at the node level.
    • Any time you do ctx.getNodeParameter('foo', itemIndex) and then call assertIsXxx('foo', foo), you’re protected against null/undefined, but you still have to guarantee that downstream code won’t do something like body.section_id = options.section as unknown. In other words, it’s easy to slip up. A small helper that returns the validated and typed value directly might reduce mistakes.

  3. Security / injection considerations
    Endpoint concatenation in GenericFunctions

    uri: `https://api.todoist.com/sync/v9${endpoint}`,

    The endpoint argument is always an internal literal in your handlers ('/quick/add' or default '/sync'), but if someone ever exposed that as a user parameter it could allow SSRF or arbitrary third-party POSTs. Consider either hard-coding the two URIs or, if you must allow more, whitelist the allowed paths.
    JSON commands in SyncHandler
    Because you allow the user to pass an arbitrary JSON string of commands, ensure that you do not inadvertently allow them to request non-Todoist APIs. In this code the only outgoing call after parsing is to your Todoist sync endpoint, so that’s safe—but worth a callout.

  4. Testing
    • You have extensive handler tests and also an end-to-end node test. Great coverage! A few gaps:
    Error states for assertIsNodeParameters missing optional cases (e.g. passing options = { durationUnit: 123 } should throw).
    FormatDueDatetime is mocked to identity; you might want at least one test of actual formatting to catch regressions there.
    QuickAdd optional fields edge cases are covered, but what about combining note + auto_reminder + an invalid reminder? You could test that it strips empty strings properly.
    • In the node harness workflow you cover one quickAdd. You could add a second test node with reminder + auto_reminder to mirror the unit tests.

  5. Minor improvements & polish
    • In MoveHandler, when nodeVersion < 2.1 you inline an IIFE just to call assertIsStringOrNumber. You could pull that logic into a tiny helper (see above).
    • You’re building a massive versionDescription block in TodoistV2.node.ts. Consider extracting each resource’s properties + operations into its own small file and importing them, for readability.
    • A couple of // eslint-disable-next-line lines remain. If you can tweak the rule config or rename the property rather than disabling, it keeps things tighter.
    • In SyncHandler.enrichProjectId, you only check CommandTypes.ITEM_ADD, but in the const you’ve added SECTION_ADD → you updated requiresProjectId to include that. Good catch, but also consider other commands that need project_id (e.g. label or reminder commands). Either whitelist or document.

  6. Consistency with n8n conventions
    • Use NodeOperationError throughout for user-facing errors.
    • Make sure every thrown error in handlers attaches the node context (this.getNode()) so the UI can highlight which operation failed.
    • Double-check your changelog / PR title formatting against the team’s convention (it looks okay but better to run your commit title linter if you have one).

Overall, this is a solid and comprehensive expansion of the Todoist node. The main areas to tighten up are reducing boilerplate with small helpers, ensuring all thrown errors become NodeOperationError, and locking down the dynamic endpoint usage. Once those are in place, you’ll have a maintainable, secure, and DRY implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants