diff --git a/package-lock.json b/package-lock.json index 8685a47..23556f0 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9,7 +9,7 @@ "version": "12.5.7", "license": "MIT", "dependencies": { - "@doist/todoist-sdk": "14.0.0", + "@doist/todoist-sdk": "14.0.1", "@modelcontextprotocol/ext-apps": "1.2.2", "date-fns": "4.1.0", "dompurify": "3.3.3", @@ -499,9 +499,9 @@ } }, "node_modules/@doist/todoist-sdk": { - "version": "14.0.0", - "resolved": "https://registry.npmjs.org/@doist/todoist-sdk/-/todoist-sdk-14.0.0.tgz", - "integrity": "sha512-dapFZq8Kf+1X+6rWucy22qvGIPWR+yQZzjXMXg6RPq9LXrI94hdtUbLNzEf1KMFJO2+l2BkvLygZWAP4JZANyg==", + "version": "14.0.1", + "resolved": "https://registry.npmjs.org/@doist/todoist-sdk/-/todoist-sdk-14.0.1.tgz", + "integrity": "sha512-l2mXCZHFkKWHyVeqGj9dZONAyOLCP5Qy89msUH4/45SDfubpFN+lx6D5Q52w2Qkc8sy6nV8rnTR7E6U7jS9myg==", "license": "MIT", "dependencies": { "camelcase": "6.3.0", diff --git a/package.json b/package.json index 8402a8c..8b0f82b 100644 --- a/package.json +++ b/package.json @@ -65,7 +65,7 @@ "prepare": "husky" }, "dependencies": { - "@doist/todoist-sdk": "14.0.0", + "@doist/todoist-sdk": "14.0.1", "@modelcontextprotocol/ext-apps": "1.2.2", "date-fns": "4.1.0", "dompurify": "3.3.3", diff --git a/src/mcp-server.ts b/src/mcp-server.ts index ab8b023..5ccfb14 100644 --- a/src/mcp-server.ts +++ b/src/mcp-server.ts @@ -42,6 +42,11 @@ What each tool does and how to fill its parameters is in the tool's own descript - To find out whether a task hides subtasks, use **fetch-object** with includeChildren rather than a speculative **find-tasks** call. - Filter tasks by label **name**. Label IDs are only for **delete-object** and **update-labels**. Shared labels can be renamed but not recoloured, reordered or favourited. +**Comments** + +- Comments notify only the people **add-comments** is handed. When a comment mentions someone, name them in \`notifyUsers\` — writing "@Ana" in the text notifies nobody. Omit \`notifyUsers\` to notify whoever the Todoist apps would, or pass \`["none"]\` to stay silent. +- Notification cannot be sent when editing a comment, only when adding one. + **Deleting and archiving** - **delete-object** removes every object type; there is no per-type delete tool. Reminders use type "reminder", location reminders "location_reminder". diff --git a/src/tool-helpers.ts b/src/tool-helpers.ts index 1585066..3563c97 100644 --- a/src/tool-helpers.ts +++ b/src/tool-helpers.ts @@ -377,6 +377,7 @@ function mapComment(comment: Comment) { content: comment.content, postedAt: comment.postedAt.toISOString(), postedUid: comment.postedUid ?? undefined, + notifiedUserIds: comment.uidsToNotify?.length ? comment.uidsToNotify : undefined, fileAttachment: comment.fileAttachment ? { resourceType: comment.fileAttachment.resourceType, diff --git a/src/tools/add-comments.test.ts b/src/tools/add-comments.test.ts index c20ebee..35262a0 100644 --- a/src/tools/add-comments.test.ts +++ b/src/tools/add-comments.test.ts @@ -1,13 +1,23 @@ -import type { Comment, TodoistApi } from '@doist/todoist-sdk' +import type { Comment, Task, TodoistApi } from '@doist/todoist-sdk' import { type Mocked, vi } from 'vitest' -import { createMockUser } from '../utils/test-helpers.js' +import { z } from 'zod' +import { ApiLimits } from '../utils/constants.js' +import { createMockTask, createMockUser } from '../utils/test-helpers.js' import { ToolNames } from '../utils/tool-names.js' +import { resolveUserRefs } from '../utils/user-resolver.js' import { addComments } from './add-comments.js' +vi.mock('../utils/user-resolver.js', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, resolveUserRefs: vi.fn() } +}) + // Mock the Todoist API const mockTodoistApi = { addComment: vi.fn(), getUser: vi.fn(), + getComments: vi.fn(), + getTask: vi.fn(), } as unknown as Mocked const { ADD_COMMENTS } = ToolNames @@ -28,10 +38,26 @@ function createMockComment(overrides: Partial = {}): Comment { } } +const CURRENT_USER_ID = 'current-user' + +function createMockTaskWithUids(overrides: Partial = {}): Task { + return { + ...createMockTask(), + addedByUid: CURRENT_USER_ID, + assignedByUid: null, + responsibleUid: null, + ...overrides, + } +} + describe(`${ADD_COMMENTS} tool`, () => { beforeEach(() => { vi.clearAllMocks() - mockTodoistApi.getUser.mockResolvedValue(createMockUser()) + mockTodoistApi.getUser.mockResolvedValue(createMockUser({ id: CURRENT_USER_ID })) + // Default: an empty thread on an unassigned, self-created task, so + // nobody is notified unless a test says otherwise. + mockTodoistApi.getComments.mockResolvedValue({ results: [], nextCursor: null }) + mockTodoistApi.getTask.mockResolvedValue(createMockTaskWithUids()) }) describe('adding comments to tasks', () => { @@ -295,6 +321,253 @@ describe(`${ADD_COMMENTS} tool`, () => { }) }) + describe('notifying users', () => { + const mockResolveUserRefs = vi.mocked(resolveUserRefs) + + function mockAddedComment(uidsToNotify: string[] | null = null) { + mockTodoistApi.addComment.mockResolvedValue( + createMockComment({ taskId: 'task456', uidsToNotify }), + ) + } + + it('should notify explicitly named users, resolving IDs, emails and names alike', async () => { + mockResolveUserRefs.mockResolvedValue([ + { userId: '111', displayName: 'Ana', email: 'ana@example.com' }, + { userId: '222', displayName: 'Bo', email: 'bo@example.com' }, + { userId: '333', displayName: 'Cleo', email: 'cleo@example.com' }, + ]) + mockAddedComment(['111', '222', '333']) + + const result = await addComments.execute( + { + comments: [ + { + taskId: 'task456', + content: 'Please review', + notifyUsers: ['111', 'bo@example.com', 'Cleo'], + }, + ], + }, + mockTodoistApi, + ) + + expect(mockResolveUserRefs).toHaveBeenCalledWith(mockTodoistApi, [ + '111', + 'bo@example.com', + 'Cleo', + ]) + expect(mockTodoistApi.addComment).toHaveBeenCalledWith({ + content: 'Please review', + taskId: 'task456', + uidsToNotify: ['111', '222', '333'], + }) + // Explicit recipients mean the thread never needs reading. + expect(mockTodoistApi.getComments).not.toHaveBeenCalled() + expect(result.textContent).toContain('Notified 3 people') + }) + + it('should surface an unresolvable user as an error', async () => { + mockResolveUserRefs.mockRejectedValue( + new Error('Could not find user(s): "Nobody", "Nobody Else".'), + ) + + await expect( + addComments.execute( + { + comments: [ + { + taskId: 'task456', + content: 'Please review', + notifyUsers: ['Nobody', 'Nobody Else'], + }, + ], + }, + mockTodoistApi, + ), + ).rejects.toThrow('Could not find user(s): "Nobody", "Nobody Else".') + }) + + it('should omit uidsToNotify entirely when told to notify nobody', async () => { + mockAddedComment() + + await addComments.execute( + { + comments: [{ taskId: 'task456', content: 'Quiet note', notifyUsers: ['none'] }], + }, + mockTodoistApi, + ) + + // Nobody to notify means no recipient field at all, rather than + // an empty one. + expect(mockTodoistApi.addComment).toHaveBeenCalledWith({ + content: 'Quiet note', + taskId: 'task456', + }) + expect(mockResolveUserRefs).not.toHaveBeenCalled() + expect(mockTodoistApi.getComments).not.toHaveBeenCalled() + }) + + it('should default a first task comment to the assignee, assigner and creator', async () => { + mockTodoistApi.getTask.mockResolvedValue( + createMockTaskWithUids({ + responsibleUid: 'assignee', + assignedByUid: 'assigner', + addedByUid: 'creator', + }), + ) + mockAddedComment(['assignee', 'assigner', 'creator']) + + await addComments.execute( + { comments: [{ taskId: 'task456', content: 'First comment' }] }, + mockTodoistApi, + ) + + expect(mockTodoistApi.addComment).toHaveBeenCalledWith({ + content: 'First comment', + taskId: 'task456', + uidsToNotify: ['assignee', 'assigner', 'creator'], + }) + }) + + it('should exclude the comment author and drop unset uids from the defaults', async () => { + mockTodoistApi.getTask.mockResolvedValue( + createMockTaskWithUids({ + responsibleUid: CURRENT_USER_ID, + assignedByUid: null, + addedByUid: 'creator', + }), + ) + mockAddedComment(['creator']) + + await addComments.execute( + { comments: [{ taskId: 'task456', content: 'First comment' }] }, + mockTodoistApi, + ) + + expect(mockTodoistApi.addComment).toHaveBeenCalledWith( + expect.objectContaining({ uidsToNotify: ['creator'] }), + ) + }) + + it("should default a reply to the previous comment's participants", async () => { + mockTodoistApi.getComments.mockResolvedValue({ + results: [ + createMockComment({ + id: 'older', + postedAt: new Date('2024-01-01T09:00:00Z'), + postedUid: 'stale-author', + uidsToNotify: ['stale-recipient'], + }), + createMockComment({ + id: 'newest', + postedAt: new Date('2024-01-01T15:00:00Z'), + postedUid: 'previous-author', + uidsToNotify: ['participant', CURRENT_USER_ID], + }), + ], + nextCursor: null, + }) + mockAddedComment(['participant', 'previous-author']) + + await addComments.execute( + { comments: [{ taskId: 'task456', content: 'Reply' }] }, + mockTodoistApi, + ) + + expect(mockTodoistApi.addComment).toHaveBeenCalledWith({ + content: 'Reply', + taskId: 'task456', + uidsToNotify: ['participant', 'previous-author'], + }) + // A thread that already has comments settles the recipients, so the + // task's own assignee/creator are never consulted. + expect(mockTodoistApi.getTask).not.toHaveBeenCalled() + }) + + it('should notify nobody on a first project comment', async () => { + mockTodoistApi.addComment.mockResolvedValue( + createMockComment({ taskId: undefined, projectId: 'project789' }), + ) + + await addComments.execute( + { comments: [{ projectId: 'project789', content: 'Project note' }] }, + mockTodoistApi, + ) + + expect(mockTodoistApi.addComment).toHaveBeenCalledWith({ + content: 'Project note', + projectId: 'project789', + }) + expect(mockTodoistApi.getTask).not.toHaveBeenCalled() + }) + + it('should read a shared thread once for several comments on the same task', async () => { + mockTodoistApi.getTask.mockResolvedValue( + createMockTaskWithUids({ responsibleUid: 'assignee' }), + ) + mockAddedComment(['assignee']) + + await addComments.execute( + { + comments: [ + { taskId: 'task456', content: 'First' }, + { taskId: 'task456', content: 'Second' }, + ], + }, + mockTodoistApi, + ) + + expect(mockTodoistApi.getComments).toHaveBeenCalledTimes(1) + expect(mockTodoistApi.addComment).toHaveBeenCalledTimes(2) + expect(mockTodoistApi.addComment).toHaveBeenCalledWith( + expect.objectContaining({ content: 'Second', uidsToNotify: ['assignee'] }), + ) + }) + + it('should surface who was notified, and omit the field when nobody was', async () => { + mockAddedComment(['111', '222']) + const notified = await addComments.execute( + { + comments: [{ taskId: 'task456', content: 'Heads up', notifyUsers: ['none'] }], + }, + mockTodoistApi, + ) + expect(notified.structuredContent?.comments[0]).toEqual( + expect.objectContaining({ notifiedUserIds: ['111', '222'] }), + ) + + mockAddedComment() + const silent = await addComments.execute( + { + comments: [{ taskId: 'task456', content: 'Quiet note', notifyUsers: ['none'] }], + }, + mockTodoistApi, + ) + expect(silent.structuredContent?.comments[0]?.notifiedUserIds).toBeUndefined() + }) + }) + + describe('notifyUsers schema bounds', () => { + const parse = (notifyUsers: unknown) => + z.object(addComments.parameters).safeParse({ + comments: [{ taskId: 'task456', content: 'x', notifyUsers }], + }) + + it('rejects an empty list, so ["none"] stays the only way to stay silent', () => { + expect(parse([]).success).toBe(false) + }) + + it('rejects more recipients than a single comment may notify', () => { + const tooMany = Array.from( + { length: ApiLimits.NOTIFY_USERS_MAX + 1 }, + (_, i) => `u${i}`, + ) + + expect(parse(tooMany).success).toBe(false) + expect(parse(tooMany.slice(0, ApiLimits.NOTIFY_USERS_MAX)).success).toBe(true) + }) + }) + describe('validation', () => { it('should throw error when neither taskId nor projectId provided', async () => { await expect( diff --git a/src/tools/add-comments.ts b/src/tools/add-comments.ts index abd6d0f..efcec6d 100644 --- a/src/tools/add-comments.ts +++ b/src/tools/add-comments.ts @@ -1,9 +1,17 @@ -import type { AddCommentArgs } from '@doist/todoist-sdk' +import type { AddCommentArgs, TodoistApi } from '@doist/todoist-sdk' import { z } from 'zod' import type { TodoistTool } from '../todoist-tool.js' import { isInboxProjectId, mapComment, resolveInboxProjectId } from '../tool-helpers.js' +import { + type CommentTarget, + NO_NOTIFY_KEYWORD, + getDefaultCommentRecipients, + isNoNotifyList, +} from '../utils/comment-recipients.js' +import { ApiLimits } from '../utils/constants.js' import { CommentSchema as CommentOutputSchema } from '../utils/output-schemas.js' import { ToolNames } from '../utils/tool-names.js' +import { resolveUserRefs } from '../utils/user-resolver.js' const CommentSchema = z.object({ taskId: z.string().optional().describe('The ID of the task to comment on.'), @@ -14,6 +22,14 @@ const CommentSchema = z.object({ 'The ID of the project to comment on. Project ID should be an ID string, or the text "inbox", for inbox tasks.', ), content: z.string().min(1).describe('The content of the comment.'), + notifyUsers: z + .array(z.string().min(1)) + .min(1) + .max(ApiLimits.NOTIFY_USERS_MAX) + .optional() + .describe( + `Who to notify about this comment — a user ID, email, full name, or "me" for each person. Set this whenever the comment mentions someone; the text of an @mention notifies nobody on its own. Omit to notify whoever the Todoist apps would (the task's assignee, assigner and creator on a first comment, or the previous comment's participants on a reply). Pass ["${NO_NOTIFY_KEYWORD}"] to notify nobody.`, + ), }) const ArgsSchema = { @@ -26,10 +42,13 @@ const OutputSchema = { addedCommentIds: z.array(z.string()).describe('The IDs of the added comments.'), } +type CommentInput = z.infer +type TodoistUser = Awaited> + const addComments = { name: ToolNames.ADD_COMMENTS, description: - 'Add multiple comments to tasks or projects. Each comment must specify either taskId or projectId.', + 'Add multiple comments to tasks or projects, optionally notifying collaborators. Each comment must specify either taskId or projectId.', parameters: ArgsSchema, outputSchema: OutputSchema, annotations: { readOnlyHint: false, destructiveHint: false, idempotentHint: false }, @@ -50,25 +69,48 @@ const addComments = { } } - // Check if any comment needs inbox resolution + // Every comment needs the current user, either to resolve "inbox" or to + // keep the author out of their own comment's recipients. const needsInboxResolution = comments.some((comment) => isInboxProjectId(comment.projectId)) - const todoistUser = needsInboxResolution ? await client.getUser() : undefined - - const addCommentPromises = comments.map(async ({ content, taskId, projectId }) => { - // Resolve "inbox" to actual inbox project ID if needed - const resolvedProjectId = await resolveInboxProjectId({ - projectId, - user: todoistUser, - client: todoistUser ? undefined : client, - }) - - return await client.addComment({ - content, - ...(taskId ? { taskId } : { projectId: resolvedProjectId }), - } as AddCommentArgs) + const needsDefaultRecipients = comments.some((comment) => !comment.notifyUsers) + const todoistUser = + needsInboxResolution || needsDefaultRecipients ? await client.getUser() : undefined + + const targets = await Promise.all( + comments.map(async (comment) => { + // Resolve "inbox" to actual inbox project ID if needed + const resolvedProjectId = await resolveInboxProjectId({ + projectId: comment.projectId, + user: todoistUser, + client: todoistUser ? undefined : client, + }) + + return ( + comment.taskId ? { taskId: comment.taskId } : { projectId: resolvedProjectId } + ) as CommentTarget + }), + ) + + const recipients = await resolveRecipientsPerComment({ + comments, + targets, + client, + currentUser: todoistUser, }) - const newComments = await Promise.all(addCommentPromises) + const newComments = await Promise.all( + comments.map(async ({ content }, index) => { + const uidsToNotify = recipients[index] ?? [] + return await client.addComment({ + content, + ...targets[index], + // Nobody to notify means no recipient field at all, + // rather than an empty one. + ...(uidsToNotify.length > 0 && { uidsToNotify }), + } as AddCommentArgs) + }), + ) + const mappedComments = newComments.map(mapComment) const textContent = generateTextContent({ comments: mappedComments }) @@ -83,6 +125,44 @@ const addComments = { }, } satisfies TodoistTool +/** + * Work out the recipients for every comment in the batch, reading each distinct + * target's thread only once. Two comments on the same task within one call are + * a single conversation, so they notify the same people. + */ +async function resolveRecipientsPerComment({ + comments, + targets, + client, + currentUser, +}: { + comments: CommentInput[] + targets: CommentTarget[] + client: TodoistApi + currentUser: TodoistUser | undefined +}): Promise { + const byTarget = new Map>() + + return await Promise.all( + comments.map(async ({ notifyUsers }, index) => { + if (notifyUsers) { + if (isNoNotifyList(notifyUsers)) return [] + const resolved = await resolveUserRefs(client, notifyUsers) + return resolved.map((user) => user.userId) + } + + const target = targets[index] + if (!target || !currentUser) return [] + + const key = target.taskId ? `task:${target.taskId}` : `project:${target.projectId}` + const pending = + byTarget.get(key) ?? getDefaultCommentRecipients(client, target, currentUser.id) + byTarget.set(key, pending) + return await pending + }), + ) +} + function generateTextContent({ comments }: { comments: ReturnType[] }): string { // Group comments by entity type and count const taskComments = comments.filter((c) => c.taskId).length @@ -100,7 +180,12 @@ function generateTextContent({ comments }: { comments: ReturnType 0 ? `Added ${parts.join(' and ')}` : 'No comments added' - return summary + const notified = new Set(comments.flatMap((c) => c.notifiedUserIds ?? [])) + if (notified.size === 0) { + return summary + } + const peopleLabel = notified.size > 1 ? 'people' : 'person' + return `${summary}. Notified ${notified.size} ${peopleLabel}` } export { addComments } diff --git a/src/utils/comment-recipients.test.ts b/src/utils/comment-recipients.test.ts new file mode 100644 index 0000000..cd68fb5 --- /dev/null +++ b/src/utils/comment-recipients.test.ts @@ -0,0 +1,219 @@ +import type { Comment, TodoistApi } from '@doist/todoist-sdk' +import { type Mocked, vi } from 'vitest' +import { + NO_NOTIFY_KEYWORD, + getDefaultCommentRecipients, + isNoNotifyList, +} from './comment-recipients.js' +import { createMockTask } from './test-helpers.js' + +const CURRENT_USER_ID = 'current-user' + +function createComment(overrides: Partial = {}): Comment { + return { + id: 'comment-1', + content: 'Existing comment', + postedAt: new Date('2024-01-01T12:00:00Z'), + postedUid: 'previous-author', + taskId: 'task-1', + projectId: undefined, + fileAttachment: null, + uidsToNotify: null, + reactions: null, + isDeleted: false, + ...overrides, + } +} + +describe('isNoNotifyList', () => { + it.each([[NO_NOTIFY_KEYWORD], ['None'], [' NONE ']])('treats %j as the opt-out', (value) => { + expect(isNoNotifyList([value])).toBe(true) + }) + + it.each([[[]], [['none', 'Ana']], [['nobody']]])('treats %j as a real list', (value) => { + expect(isNoNotifyList(value)).toBe(false) + }) +}) + +describe('getDefaultCommentRecipients', () => { + let mockClient: Mocked + + beforeEach(() => { + mockClient = { + getComments: vi.fn().mockResolvedValue({ results: [], nextCursor: null }), + getTask: vi.fn().mockResolvedValue(createMockTask()), + } as unknown as Mocked + }) + + describe('a thread that already has comments', () => { + it("takes the newest comment's participants, not the first page's", async () => { + mockClient.getComments.mockResolvedValue({ + results: [ + createComment({ + id: 'newest', + postedAt: new Date('2024-03-01T09:00:00Z'), + postedUid: 'recent-author', + uidsToNotify: ['recent-participant'], + }), + createComment({ + id: 'oldest', + postedAt: new Date('2024-01-01T09:00:00Z'), + postedUid: 'stale-author', + uidsToNotify: ['stale-participant'], + }), + ], + nextCursor: null, + }) + + const recipients = await getDefaultCommentRecipients( + mockClient, + { taskId: 'task-1' }, + CURRENT_USER_ID, + ) + + expect(recipients).toEqual(['recent-participant', 'recent-author']) + expect(mockClient.getTask).not.toHaveBeenCalled() + }) + + it('keeps the chain alive from a comment that notified nobody', async () => { + mockClient.getComments.mockResolvedValue({ + results: [createComment({ postedUid: 'agent', uidsToNotify: null })], + nextCursor: null, + }) + + const recipients = await getDefaultCommentRecipients( + mockClient, + { taskId: 'task-1' }, + CURRENT_USER_ID, + ) + + expect(recipients).toEqual(['agent']) + }) + + it('excludes the author of the comment being posted', async () => { + mockClient.getComments.mockResolvedValue({ + results: [ + createComment({ + postedUid: CURRENT_USER_ID, + uidsToNotify: [CURRENT_USER_ID, 'other'], + }), + ], + nextCursor: null, + }) + + const recipients = await getDefaultCommentRecipients( + mockClient, + { taskId: 'task-1' }, + CURRENT_USER_ID, + ) + + expect(recipients).toEqual(['other']) + }) + }) + + describe('a task with no comments yet', () => { + it('takes the assignee, assigner and creator', async () => { + mockClient.getTask.mockResolvedValue( + createMockTask({ + responsibleUid: 'assignee', + assignedByUid: 'assigner', + addedByUid: 'creator', + }), + ) + + const recipients = await getDefaultCommentRecipients( + mockClient, + { taskId: 'task-1' }, + CURRENT_USER_ID, + ) + + expect(recipients).toEqual(['assignee', 'assigner', 'creator']) + }) + + it('drops unset uids and collapses a person filling two roles', async () => { + mockClient.getTask.mockResolvedValue( + createMockTask({ + responsibleUid: 'ana', + assignedByUid: null, + addedByUid: 'ana', + }), + ) + + const recipients = await getDefaultCommentRecipients( + mockClient, + { taskId: 'task-1' }, + CURRENT_USER_ID, + ) + + expect(recipients).toEqual(['ana']) + }) + + it('notifies nobody when the author is the only party', async () => { + mockClient.getTask.mockResolvedValue( + createMockTask({ + responsibleUid: CURRENT_USER_ID, + assignedByUid: null, + addedByUid: CURRENT_USER_ID, + }), + ) + + const recipients = await getDefaultCommentRecipients( + mockClient, + { taskId: 'task-1' }, + CURRENT_USER_ID, + ) + + expect(recipients).toEqual([]) + }) + }) + + it('walks every page of a long thread to reach the newest comment', async () => { + mockClient.getComments + .mockResolvedValueOnce({ + results: [ + createComment({ postedAt: new Date('2024-01-01T09:00:00Z'), postedUid: 'p1' }), + ], + nextCursor: 'page-2', + }) + .mockResolvedValueOnce({ + results: [ + createComment({ postedAt: new Date('2024-02-01T09:00:00Z'), postedUid: 'p2' }), + ], + nextCursor: 'page-3', + }) + .mockResolvedValueOnce({ + results: [ + createComment({ postedAt: new Date('2024-03-01T09:00:00Z'), postedUid: 'p3' }), + ], + nextCursor: null, + }) + + const recipients = await getDefaultCommentRecipients( + mockClient, + { taskId: 'task-1' }, + CURRENT_USER_ID, + ) + + expect(mockClient.getComments).toHaveBeenCalledTimes(3) + expect(mockClient.getComments).toHaveBeenLastCalledWith( + expect.objectContaining({ cursor: 'page-3' }), + ) + expect(recipients).toEqual(['p3']) + }) + + describe('a project with no comments yet', () => { + it('notifies nobody, as a project has no assignee to fall back on', async () => { + const recipients = await getDefaultCommentRecipients( + mockClient, + { projectId: 'project-1' }, + CURRENT_USER_ID, + ) + + expect(mockClient.getComments).toHaveBeenCalledWith( + expect.objectContaining({ projectId: 'project-1' }), + ) + expect(recipients).toEqual([]) + expect(mockClient.getTask).not.toHaveBeenCalled() + }) + }) +}) diff --git a/src/utils/comment-recipients.ts b/src/utils/comment-recipients.ts new file mode 100644 index 0000000..6ee9901 --- /dev/null +++ b/src/utils/comment-recipients.ts @@ -0,0 +1,88 @@ +import type { Comment, GetTaskCommentsArgs, TodoistApi } from '@doist/todoist-sdk' + +/** Value that tells `add-comments` to post without notifying anyone. */ +export const NO_NOTIFY_KEYWORD = 'none' + +export type CommentTarget = + | { taskId: string; projectId?: never } + | { projectId: string; taskId?: never } + +/** Whether a `notifyUsers` list is the explicit "notify nobody" opt-out. */ +export function isNoNotifyList(notifyUsers: string[]): boolean { + return notifyUsers.length === 1 && notifyUsers[0]?.trim().toLowerCase() === NO_NOTIFY_KEYWORD +} + +/** + * Work out who Todoist's own clients would notify about a new comment. + * + * The API notifies exactly the people it is handed and derives nobody itself, + * so a comment posted without recipients notifies no one — and, because a reply + * takes its recipients from the comment before it, silences the next comment in + * the thread too. Mirroring the clients here keeps that chain intact: + * + * - replying to an existing thread notifies the previous comment's participants + * - the first comment on a task notifies its assignee, assigner and creator + * - the first comment on a project notifies nobody, as a project has no assignee + * + * The comment's own author is never a recipient. + * + * @param client - Todoist API client + * @param target - The task or project being commented on + * @param currentUserId - The authenticated user, excluded from the result + * @returns User IDs to notify, deduplicated + */ +export async function getDefaultCommentRecipients( + client: TodoistApi, + target: CommentTarget, + currentUserId: string, +): Promise { + const previousComment = await getLatestComment(client, target) + + if (previousComment) { + return dedupe( + [...(previousComment.uidsToNotify ?? []), previousComment.postedUid], + currentUserId, + ) + } + + if (!target.taskId) { + return [] + } + + const task = await client.getTask(target.taskId) + return dedupe([task.responsibleUid, task.assignedByUid, task.addedByUid], currentUserId) +} + +async function getLatestComment( + client: TodoistApi, + target: CommentTarget, +): Promise { + let latest: Comment | undefined + let cursor: string | null = null + + // Walked a page at a time, keeping only the newest comment seen. A thread + // can be arbitrarily long and all we want from it is its last participant, + // so there is no reason to hold the whole history in memory. Page order is + // not guaranteed, so every page is still compared. + do { + const response = await client.getComments({ + ...(target.taskId ? { taskId: target.taskId } : { projectId: target.projectId }), + cursor, + } as GetTaskCommentsArgs) + + for (const comment of response.results) { + if (!latest || comment.postedAt > latest.postedAt) latest = comment + } + cursor = response.nextCursor + } while (cursor) + + return latest +} + +function dedupe(userIds: (string | null | undefined)[], currentUserId: string): string[] { + const seen = new Set() + for (const userId of userIds) { + if (userId && userId !== currentUserId) seen.add(userId) + } + return [...seen] +} diff --git a/src/utils/constants.ts b/src/utils/constants.ts index 317090c..73bb4f8 100644 --- a/src/utils/constants.ts +++ b/src/utils/constants.ts @@ -29,6 +29,8 @@ export const ApiLimits = { COMMENTS_DEFAULT: 10, /** Maximum limit for comment search and list operations */ COMMENTS_MAX: 10, + /** Maximum number of users a single comment can notify */ + NOTIFY_USERS_MAX: 25, /** Default limit for activity log listings */ ACTIVITY_DEFAULT: 20, /** Maximum limit for activity log search and list operations */ diff --git a/src/utils/output-schemas.ts b/src/utils/output-schemas.ts index 5088b16..1119b2b 100644 --- a/src/utils/output-schemas.ts +++ b/src/utils/output-schemas.ts @@ -98,6 +98,10 @@ const CommentSchema = z.object({ content: z.string(), postedAt: z.string().describe('ISO 8601.'), postedUid: z.string().optional(), + notifiedUserIds: z + .array(z.string()) + .optional() + .describe('Users notified about this comment. Absent when nobody was notified.'), fileAttachment: AttachmentSchema.optional(), }) diff --git a/src/utils/user-resolver.test.ts b/src/utils/user-resolver.test.ts index f7d6d12..822af3c 100644 --- a/src/utils/user-resolver.test.ts +++ b/src/utils/user-resolver.test.ts @@ -1,6 +1,12 @@ import type { TodoistApi } from '@doist/todoist-sdk' import { type Mocked, vi } from 'vitest' -import { BoundedTtlCache, SELF_USER_KEYWORD, UserResolver } from './user-resolver.js' +import { + BoundedTtlCache, + SELF_USER_KEYWORD, + UserResolver, + resolveUserRefs, + userResolver, +} from './user-resolver.js' describe('BoundedTtlCache', () => { it('evicts the least recently used entry at capacity', () => { @@ -252,3 +258,70 @@ describe('UserResolver', () => { }) }) }) + +describe('resolveUserRefs', () => { + const collaborators = [ + { id: '111', name: 'Ana Lovelace', email: 'ana@example.com' }, + { id: '222', name: 'Bo Turing', email: 'bo@example.com' }, + ] + + let mockClient: Mocked + + beforeEach(() => { + userResolver.clearCache() + mockClient = { + getUser: vi + .fn() + .mockResolvedValue({ id: '999', fullName: 'Me', email: 'me@example.com' }), + getProjects: vi + .fn() + .mockResolvedValue({ results: [{ id: 'p1', isShared: true }], nextCursor: null }), + getProjectCollaborators: vi + .fn() + .mockResolvedValue({ results: collaborators, nextCursor: null }), + } as unknown as Mocked + }) + + it('resolves IDs, emails, names and "me" in one pass', async () => { + const resolved = await resolveUserRefs(mockClient, ['111', 'bo@example.com', 'me']) + + expect(resolved.map((user) => user.userId)).toEqual(['111', '222', '999']) + }) + + it('preserves input order and collapses duplicates', async () => { + const resolved = await resolveUserRefs(mockClient, [ + 'Bo Turing', + 'ana@example.com', + 'bo@example.com', + ]) + + expect(resolved.map((user) => user.userId)).toEqual(['222', '111']) + }) + + it('names every unresolvable reference in a single error', async () => { + await expect( + resolveUserRefs(mockClient, ['Ana Lovelace', 'Ghost', 'Phantom']), + ).rejects.toThrow( + 'Could not find user(s): "Ghost", "Phantom". Make sure they are collaborators on a shared project.', + ) + }) + + it('resolves nothing for an empty list', async () => { + await expect(resolveUserRefs(mockClient, [])).resolves.toEqual([]) + }) + + it('looks a repeated reference up only once', async () => { + await resolveUserRefs(mockClient, ['Ana Lovelace', 'ana lovelace', ' Ana Lovelace ']) + + // The collaborator lookup is shared, so a repeated name must not send + // the whole project list off to be fetched again. + expect(mockClient.getProjects).toHaveBeenCalledTimes(1) + }) + + it('reuses the warmed collaborator cache across distinct references', async () => { + await resolveUserRefs(mockClient, ['Ana Lovelace', 'Bo Turing']) + + expect(mockClient.getProjects).toHaveBeenCalledTimes(1) + expect(mockClient.getProjectCollaborators).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/utils/user-resolver.ts b/src/utils/user-resolver.ts index 3ea8d87..d125b8b 100644 --- a/src/utils/user-resolver.ts +++ b/src/utils/user-resolver.ts @@ -381,3 +381,52 @@ export async function resolveUserNameToId( ): Promise { return userResolver.resolveUser(client, nameOrId) } + +/** + * Resolve a list of user references — IDs, emails, full names, or the "me" + * keyword — to users, in one pass. + * + * Order of the input is preserved and duplicates are collapsed, so callers can + * hand the result straight to an API that expects a recipient list. Every + * reference that cannot be resolved is reported in a single error rather than + * failing on the first one, so the caller learns about all the bad references + * at once. + * + * @param client - Todoist API client + * @param refs - User references to resolve + * @returns The resolved users, deduplicated by user ID + * @throws Error naming every reference that could not be resolved + */ +export async function resolveUserRefs(client: TodoistApi, refs: string[]): Promise { + const seenRefs = new Set() + const seenUserIds = new Set() + const resolved: ResolvedUser[] = [] + const unresolved: string[] = [] + + // Resolved one at a time on purpose: the first lookup populates the + // collaborator cache that the rest read, where resolving in parallel would + // send every reference through its own full collaborator fetch. + for (const ref of refs) { + const dedupeKey = ref.trim().toLowerCase() + if (seenRefs.has(dedupeKey)) continue + seenRefs.add(dedupeKey) + + const user = await userResolver.resolveUser(client, ref) + if (!user) { + unresolved.push(ref) + continue + } + if (seenUserIds.has(user.userId)) continue + seenUserIds.add(user.userId) + resolved.push(user) + } + + if (unresolved.length > 0) { + const names = unresolved.map((ref) => `"${ref}"`).join(', ') + throw new Error( + `Could not find user(s): ${names}. Make sure they are collaborators on a shared project.`, + ) + } + + return resolved +}