Integrate template tools #31
Conversation
…from environment variables
…ns and add MAILTRAP_ACCOUNT_ID to configuration examples
Implemented create, list, update, and delete functionalities for email templates, including their respective schemas. Updated the server tool configuration to support these new operations.
…date Implemented four new functions for managing email templates in the Mailtrap client: createTemplate, deleteTemplate, listTemplates, and updateTemplate. Each function handles success and error responses appropriately, enhancing the overall template management capabilities.
Created an index file to consolidate exports for email template management functions, including create, list, update, and delete operations along with their respective schemas. This enhances module organization and simplifies imports in other parts of the application.
Implemented comprehensive unit tests for createTemplate, deleteTemplate, listTemplates, and updateTemplate functions. Each test suite includes scenarios for successful operations and error handling, ensuring robust validation of template management functionalities.
Created new schemas for creating, deleting, listing, and updating email templates using Zod for validation. This addition enhances the structure and validation of template management functionalities.
Introduced new interfaces for creating, updating, and deleting email templates in the Mailtrap client. This addition enhances the structure and type safety for template management operations, aligning with the recent functionalities implemented for email templates.
WalkthroughAdds Mailtrap template CRUD support: new Zod schemas, handler implementations, types and tests; registers four template tools on the server; README/CLAUDE/AGENT docs updated to include templates and MAILTRAP_ACCOUNT_ID; Mailtrap client now conditionally uses numeric accountId from env. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Server
participant TemplatesModule
participant MailtrapAPI
User->>Server: call tool (create-template / list-templates / update-template / delete-template)
Server->>Server: validate input (Zod)
Server->>TemplatesModule: invoke handler with validated payload
TemplatesModule->>MailtrapAPI: client.templates.<action>(payload)
MailtrapAPI-->>TemplatesModule: returns success / error
TemplatesModule-->>Server: { content: [...], isError? }
Server-->>User: return response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (21)
src/types/mailtrap.ts (1)
20-27: Consider TS naming consistency for IDsUsing snake_case (
template_id) in TypeScript interfaces is uncommon compared to camelCase. If feasible, prefertemplateIdin the TS layer and map totemplate_idat the API boundary to keep TS idiomatic and API-specific casing encapsulated.If you want to adopt this now, here’s a focused change (interface-only; you’d need to adjust callers accordingly):
export interface UpdateTemplateRequest { - template_id: number; + templateId: number; name?: string; subject?: string; html?: string; text?: string; category?: string; } export interface DeleteTemplateRequest { - template_id: number; + templateId: number; }Also applies to: 29-31
src/tools/templates/__tests__/createTemplate.test.ts (2)
34-54: Nit: test name doesn’t match payload (includes optional text)The case includes
text, so it’s not strictly “required fields.” Consider a clearer name.- it("should create template successfully with all required fields", async () => { + it("should create template successfully with required fields and optional text", async () => {
93-99: Prefer not asserting explicitundefinedproperty in payloadPassing
body_text: undefinedis typically avoided for API payloads; omit the key whentextis not provided and adapt the assertion to be more resilient.- expect(client.templates.create).toHaveBeenCalledWith({ - name: mockTemplateData.name, - subject: mockTemplateData.subject, - category: mockTemplateData.category, - body_html: mockTemplateData.html, - body_text: undefined, - }); + const payload = (client.templates.create as jest.Mock).mock.calls[0][0]; + expect(payload).toMatchObject({ + name: mockTemplateData.name, + subject: mockTemplateData.subject, + category: mockTemplateData.category, + body_html: mockTemplateData.html, + }); + expect(payload).not.toHaveProperty("body_text");src/tools/templates/deleteTemplate.ts (2)
4-7: Return type: avoidany[]; consider a shared ToolResponse typeCurrent signature leaks
any[]. If you have a common tool response shape, use it here for consistency and stronger typing.-}: DeleteTemplateRequest): Promise<{ content: any[]; isError?: boolean }> { +}: DeleteTemplateRequest): Promise<{ content: { type: "text"; text: string }[]; isError?: boolean }> {If a shared
ToolResponsetype exists elsewhere, import and use that instead.
18-21: Consider using a logger instead of console.errorUse your project’s logging facility to control verbosity and destinations.
src/tools/templates/createTemplate.ts (3)
15-15: Default category: use nullish coalescing to avoid overriding empty strings
||treats empty strings as falsy. If empty string is a valid category, prefer??.- category: category || "General", + category: category ?? "General",
12-18: Optionally omitbody_textwhentextis undefinedSome APIs prefer missing keys over explicit
undefined. Build the payload conditionally.- const template = await client.templates.create({ - name, - subject, - category: category || "General", - body_html: html, - body_text: text, - }); + const payload: Record<string, unknown> = { + name, + subject, + category: category ?? "General", + body_html: html, + }; + if (typeof text !== "undefined") payload.body_text = text; + const template = await client.templates.create(payload);
29-29: Consider switching to project logger over console.errorKeeps logging consistent and configurable.
src/tools/templates/__tests__/deleteTemplate.test.ts (1)
15-18: Nit: also assert call counts and success-case flagsConsider minor assertions to strengthen the tests:
- Ensure the delete method is called exactly once per invocation.
- Assert that
isErroris not set on success.Example:
expect(client.templates.delete).toHaveBeenCalledTimes(1); expect(result.isError).toBeUndefined();Also applies to: 20-24, 35-41
src/tools/templates/listTemplates.ts (2)
3-3: Avoidany[]in the return type; define a shared ToolResponseUse a concrete response type for consistency and type safety.
-async function listTemplates(): Promise<{ content: any[]; isError?: boolean }> { +type ToolResponse = { + content: Array<{ type: "text"; text: string }>; + isError?: boolean; +}; +async function listTemplates(): Promise<ToolResponse> {Also applies to: 25-33, 38-47
18-23: Guard against missing fields to avoid rendering “undefined”If any of subject/category/created_at might be absent, provide fallbacks to keep output clean.
- (template) => - `• ${template.name} (ID: ${template.id}, UUID: ${template.uuid})\n Subject: ${template.subject}\n Category: ${template.category}\n Created: ${template.created_at}\n` + (template) => + `• ${template.name} (ID: ${template.id}, UUID: ${template.uuid})\n` + + ` Subject: ${template.subject ?? "N/A"}\n` + + ` Category: ${template.category ?? "N/A"}\n` + + ` Created: ${template.created_at ?? "N/A"}\n`src/tools/templates/__tests__/listTemplates.test.ts (1)
44-77: If adding field fallbacks, update expectationsIf you adopt “N/A” fallbacks for optional fields in the handler, update expected strings accordingly (subject/category/created_at lines).
Also applies to: 130-154
README.md (2)
106-119: Call out env requirement for template toolsAdd a brief note that template tools require
MAILTRAP_ACCOUNT_IDto be set, so users understand why listing/updating/deleting may fail otherwise.
208-210: Ensure consistency: update all quick-install/env examples to include MAILTRAP_ACCOUNT_IDThese sections correctly include the var. Consider also updating earlier “Install with Node in VS Code” and Cursor badges’ env blocks to include
MAILTRAP_ACCOUNT_IDfor completeness.Also applies to: 232-234, 254-256
src/tools/templates/updateTemplate.ts (3)
13-19: Avoidanyand build a typed payloadDefine a narrow type for the Mailtrap update payload and remove
any.- const updateData: any = {}; - if (name !== undefined) updateData.name = name; - if (subject !== undefined) updateData.subject = subject; - if (html !== undefined) updateData.body_html = html; - if (text !== undefined) updateData.body_text = text; - if (category !== undefined) updateData.category = category; + type TemplateUpdatePayload = { + name?: string; + subject?: string; + body_html?: string; + body_text?: string; + category?: string; + }; + const updateData: TemplateUpdatePayload = {}; + if (name !== undefined) updateData.name = name; + if (subject !== undefined) updateData.subject = subject; + if (html !== undefined) updateData.body_html = html; + if (text !== undefined) updateData.body_text = text; + if (category !== undefined) updateData.category = category;
33-43: Add context (template_id) to error messagesIncluding the
template_idaids debugging.- text: `Failed to update template: ${errorMessage}`, + text: `Failed to update template ${template_id}: ${errorMessage}`,
11-11: Unify response typing across toolsReturn type uses
any[]. Consider introducing a shared ToolResponse (as suggested in listTemplates) and use it here for consistency.-}: UpdateTemplateRequest): Promise<{ content: any[]; isError?: boolean }> { +}: UpdateTemplateRequest): Promise<ToolResponse> {Also applies to: 22-29, 35-43
src/index.ts (2)
37-63: Optional: DRY up repetitive tool registrationsConsider registering with a small descriptor array to reduce repetition and ease future additions.
-/** - * Templates operations. - */ -server.tool( - "create-template", - "Create a new email template", - createTemplateSchema, - createTemplate -); -server.tool( - "list-templates", - "List all email templates", - listTemplatesSchema, - listTemplates -); -server.tool( - "update-template", - "Update an existing email template", - updateTemplateSchema, - updateTemplate -); -server.tool( - "delete-template", - "Delete an existing email template", - deleteTemplateSchema, - deleteTemplate -); +/** + * Template operations. + */ +const templateTools = [ + { name: "create-template", desc: "Create a new email template", schema: createTemplateSchema, handler: createTemplate }, + { name: "list-templates", desc: "List all email templates", schema: listTemplatesSchema, handler: listTemplates }, + { name: "update-template", desc: "Update an existing email template", schema: updateTemplateSchema, handler: updateTemplate }, + { name: "delete-template", desc: "Delete an existing email template", schema: deleteTemplateSchema, handler: deleteTemplate }, +]; +templateTools.forEach(t => server.tool(t.name, t.desc, t.schema, t.handler));
37-39: Nit: Comment wordingUse “Template operations” (singular adjective) instead of “Templates operations.”
-/** - * Templates operations. - */ +/** + * Template operations. + */src/tools/templates/__tests__/updateTemplate.test.ts (2)
36-55: Align success message expectation with API response (consistency)In this test you use the input name, while other tests assert using mockResponse.name. Prefer consistently asserting on the response-derived value.
- text: `Template "${mockUpdateData.name}" updated successfully!\nTemplate ID: ${mockResponse.id}\nTemplate UUID: ${mockResponse.uuid}`, + text: `Template "${mockResponse.name}" updated successfully!\nTemplate ID: ${mockResponse.id}\nTemplate UUID: ${mockResponse.uuid}`,
36-214: Optional: Add call count assertions and DRY helpers
- Add expect(client.templates.update).toHaveBeenCalledTimes(1) in each test to catch accidental multiple invocations.
- Extract repeated success message and call assertions into helpers to reduce duplication and ease updates.
Example:
const successText = (name = mockResponse.name) => `Template "${name}" updated successfully!\nTemplate ID: ${mockResponse.id}\nTemplate UUID: ${mockResponse.uuid}`; const expectUpdated = (id: number, payload: object) => { expect(client.templates.update).toHaveBeenCalledTimes(1); expect(client.templates.update).toHaveBeenCalledWith(id, payload); };Then use:
expectUpdated(mockTemplateId, { name: "New Template Name" }); expect(result).toEqual({ content: [{ type: "text", text: successText() }] });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
README.md(6 hunks)src/client.ts(1 hunks)src/index.ts(2 hunks)src/tools/templates/__tests__/createTemplate.test.ts(1 hunks)src/tools/templates/__tests__/deleteTemplate.test.ts(1 hunks)src/tools/templates/__tests__/listTemplates.test.ts(1 hunks)src/tools/templates/__tests__/updateTemplate.test.ts(1 hunks)src/tools/templates/createTemplate.ts(1 hunks)src/tools/templates/deleteTemplate.ts(1 hunks)src/tools/templates/index.ts(1 hunks)src/tools/templates/listTemplates.ts(1 hunks)src/tools/templates/schemas/createTemplate.ts(1 hunks)src/tools/templates/schemas/deleteTemplate.ts(1 hunks)src/tools/templates/schemas/listTemplates.ts(1 hunks)src/tools/templates/schemas/updateTemplate.ts(1 hunks)src/tools/templates/updateTemplate.ts(1 hunks)src/types/mailtrap.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/tools/templates/updateTemplate.ts (2)
src/types/mailtrap.ts (1)
UpdateTemplateRequest(20-27)src/client.ts (1)
client(19-19)
src/tools/templates/createTemplate.ts (3)
src/tools/templates/index.ts (1)
createTemplate(12-12)src/types/mailtrap.ts (1)
CreateTemplateRequest(12-18)src/client.ts (1)
client(19-19)
src/index.ts (2)
src/tools/sendEmail/index.ts (2)
sendEmailSchema(4-4)sendEmail(4-4)src/tools/templates/index.ts (8)
createTemplateSchema(11-11)createTemplate(12-12)listTemplatesSchema(13-13)listTemplates(14-14)updateTemplateSchema(15-15)updateTemplate(16-16)deleteTemplateSchema(17-17)deleteTemplate(18-18)
🪛 LanguageTool
README.md
[style] ~110-~110: Consider using “email”.
Context: ...for example: Email Operations: - "Send an email to john.doe@example.com with the subject '...
(SEND_AN_EMAIL)
🪛 markdownlint-cli2 (0.17.2)
README.md
110-110: Bare URL used
(MD034, no-bare-urls)
111-111: Bare URL used
(MD034, no-bare-urls)
111-111: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (12)
src/types/mailtrap.ts (1)
12-18: CreateTemplateRequest looks goodShape aligns with the intended payload (name, subject, html required; text/category optional).
src/tools/templates/schemas/listTemplates.ts (1)
1-5: No change required: listTemplatesSchema can remain an empty shape objectThe MCP server’s tool method accepts plain “shape” objects (it internally wraps them into a Zod schema), as evidenced by the existing createTemplateSchema, updateTemplateSchema, and deleteTemplateSchema. Keeping listTemplatesSchema as an empty object is correct.
- src/tools/templates/schemas/listTemplates.ts: retain
const listTemplatesSchema = { // No parameters needed for listing templates }; export default listTemplatesSchema;src/tools/templates/schemas/createTemplate.ts (1)
1-17: Prefer z.object and enforce non-empty/default valuesWrap your shape in
z.objectto produce an actual ZodObject (required by most registry tools) and add minimal constraints/defaults:File: src/tools/templates/schemas/createTemplate.ts
import { z } from "zod"; -const createTemplateSchema = { - name: z.string().describe("Name of the template"), - subject: z.string().describe("Email subject line"), - html: z.string().describe("HTML content of the template"), - text: z - .string() - .optional() - .describe("Plain text version of the template (optional)"), - category: z - .string() - .optional() - .describe("Template category (optional, defaults to 'General')"), -}; +const createTemplateSchema = z.object({ + name: z.string().min(1).describe("Name of the template"), + subject: z.string().min(1).describe("Email subject line"), + html: z.string().min(1).describe("HTML content of the template"), + text: z.string().min(1).optional().describe("Plain text version of the template (optional)"), + category: z.string().min(1).default("General") + .describe("Template category (optional, defaults to 'General')"), +}); export default createTemplateSchema;• Ensure
McpServer.tool(...)in src/index.ts accepts a ZodObject rather than a bare shape.
• If it expects a plain “shape,” still apply the.min(1)and.default("General")changes above.
• Please verify in your MCP SDK that the schema parameter is defined asz.ZodObject<…>and adjust if necessary.src/tools/templates/index.ts (1)
1-19: LGTM — clear single entry point for template toolsRe-exports are concise and consistent; paths look correct.
src/tools/templates/createTemplate.ts (1)
12-18: LGTM — correct client mapping and default categoryPayload mapping looks correct; happy path response is well-formed.
src/tools/templates/__tests__/deleteTemplate.test.ts (1)
20-33: Coverage and behavior validation look solidGood use of mocking, multiple IDs, and error-shape coverage. Assertions on exact output help prevent regressions.
src/tools/templates/__tests__/listTemplates.test.ts (1)
44-77: Excellent breadth of cases and precise formatting checksThe suite thoroughly validates normal, empty, and error scenarios. Mocks are cleanly isolated per test.
Also applies to: 79-94, 96-111, 113-128, 130-154, 156-206
src/tools/templates/updateTemplate.ts (1)
13-21: Guard: no fields provided should be an error (unless schema enforces it)If the schema doesn’t enforce “at least one of name/subject/html/text/category,” avoid sending an empty update payload.
if (category !== undefined) updateData.category = category; + if (Object.keys(updateData).length === 0) { + return { + content: [ + { + type: "text", + text: + "No fields provided to update. Provide at least one of: name, subject, html, text, category.", + }, + ], + isError: true, + }; + }Do you want this guard in the handler, or is it already enforced in
updateTemplateSchema? If enforced, we can skip this.src/index.ts (2)
40-63: Tool registrations look correct and consistentNames, descriptions, schemas, and handlers are aligned and consistently kebab-cased. Good separation between sending and template operations.
5-6: Reorder dotenv import isn’t needed—CONFIGdoesn’t useprocess.env
A review ofsrc/config/index.tsshows it exports only static values and never readsprocess.env. Since there’s no environment lookup at module load, reordering or switching toimport "dotenv/config"isn’t necessary. You can safely ignore the suggested change.Likely an incorrect or invalid review comment.
src/tools/templates/__tests__/updateTemplate.test.ts (2)
12-214: Solid coverage of update paths and error handlingGood breadth: all individual fields, multiple fields, different ID, and four error scenarios. Payload mapping checks (html→body_html, text→body_text) are precise.
1-11: Verify Jest mock hoisting for client importThe top-level
jest.mock("../../../client")relies on Jest’s automatic hoisting (via ts-jest or babel-jest). If your tests ever run under native ESM without a transformer, the real module could be imported before the mock is applied.• Confirm your Jest config (jest.config.js or package.json “jest” field) includes a
transformfor TypeScript (e.g. ts-jest) or Babel.
• If you prefer not to depend on hoisting, switch to a dynamic mock:jest.doMock("../../../client", () => ({ client: { templates: { update: jest.fn() } } })); const { default: updateTemplate } = await import("../updateTemplate"); const { client } = await import("../../../client");
…_ACCOUNT_ID" in configuration examples.
… as a number before setting accountId from environment variables
…ne update field is provided, enhancing error handling for template updates.
… provided This test ensures that the updateTemplate function correctly handles cases where no update fields are supplied, returning an appropriate error message and preventing the update operation from being called.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
README.md (1)
53-55: Typo fixed: MAILTRAP_ACCOUNT_IDGood catch correcting MAILTRA_ACCOUNT_ID → MAILTRAP_ACCOUNT_ID. This resolves the earlier config breakage.
🧹 Nitpick comments (5)
README.md (5)
53-55: Clarify MAILTRAP_ACCOUNT_ID usage (optional + numeric)Recommend adding a short note here that MAILTRAP_ACCOUNT_ID is optional and must be a numeric ID. Also consider adding this key to the earlier non-asdf example for consistency across all config blocks.
149-156: Consider documenting pagination/filters (if supported)If the underlying API supports pagination or filtering, expose optional params (page, limit, search) here to set expectations. If not supported, explicitly state that the full list is returned.
208-210: Mailtrap account ID note (optional + numeric)Same as earlier: add a note that MAILTRAP_ACCOUNT_ID is optional and must be a numeric ID.
232-234: Repeat the optional/numeric note for consistencyRecommend adding the “optional + numeric” clarification here too, to keep all config blocks aligned.
254-256: Repeat the optional/numeric note for consistencyRecommend adding the “optional + numeric” clarification here too, to keep all config blocks aligned.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(6 hunks)src/client.ts(1 hunks)src/tools/templates/__tests__/updateTemplate.test.ts(1 hunks)src/tools/templates/updateTemplate.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/tools/templates/tests/updateTemplate.test.ts
- src/tools/templates/updateTemplate.ts
- src/client.ts
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~110-~110: Consider using “email”.
Context: ...for example: Email Operations: - "Send an email to john.doe@example.com with the subject '...
(SEND_AN_EMAIL)
🪛 markdownlint-cli2 (0.17.2)
README.md
110-110: Bare URL used
(MD034, no-bare-urls)
111-111: Bare URL used
(MD034, no-bare-urls)
111-111: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (3)
README.md (3)
106-119: Usage expansion looks goodClear, helpful examples covering both email and template operations.
137-148: create-template docs are clearParameters and defaults are well explained.
170-177: delete-template docs LGTMClear requirement for template_id.
Added a note emphasizing that at least one updatable field must be provided when calling the update-template function, enhancing user understanding of the update process.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
README.md (2)
131-132: Docs require text or html; verify schema enforces it in sendEmailSchemaThe README states one of text/html is required. Please confirm the Zod schema enforces “at least one of text or html” (e.g., via refine/superRefine) to avoid invalid payloads.
Run to verify:
#!/bin/bash # Verify sendEmail schema enforces at-least-one(text, html) fd schema.ts | rg 'src/tools/sendEmail/schema.ts' || true rg -n "sendEmailSchema" src/tools/sendEmail/schema.ts -n -A5 -B5 || true rg -n "refine|superRefine" src/tools/sendEmail/schema.ts -n -A3 -B3 || true rg -n "text" src/tools/sendEmail/schema.ts -n -A3 -B3 || true rg -n "html" src/tools/sendEmail/schema.ts -n -A3 -B3 || true
157-172: Good addition: require at least one updatable fieldThe note preventing no-op updates is clear and addresses prior feedback.
🧹 Nitpick comments (4)
README.md (4)
53-54: Typo fix confirmed; consider clarifying when MAILTRAP_ACCOUNT_ID is neededThe environment variable name is correctly spelled now. Suggest adding a short note that MAILTRAP_ACCOUNT_ID is optional and only required for accounts/projects where Mailtrap expects an accountId (to reduce confusion for users who don’t need it).
211-213: Env block LGTM; consider an “optional” note for MAILTRAP_ACCOUNT_IDSame suggestion as above: clarify that account ID is optional and only needed when applicable to the user’s Mailtrap setup.
235-237: Env block (asdf path) LGTM; add optionality note for account IDConsistent with other sections, consider a one-line note that MAILTRAP_ACCOUNT_ID may not be required for all users.
257-259: VS Code env block LGTM; add optionality note for account IDSuggest adding a brief clarification on when MAILTRAP_ACCOUNT_ID is needed to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(6 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~110-~110: Consider using “email”.
Context: ...for example: Email Operations: - "Send an email to john.doe@example.com with the subject '...
(SEND_AN_EMAIL)
🪛 markdownlint-cli2 (0.17.2)
README.md
110-110: Bare URL used
(MD034, no-bare-urls)
111-111: Bare URL used
(MD034, no-bare-urls)
111-111: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (2)
README.md (2)
106-119: Usage examples: templates coverage looks goodNice addition of template operation examples alongside send-email usage. Clear and actionable.
137-148: Category default is correctly implemented
ThecreateTemplatehandler appliescategory || "General"when callingclient.templates.create, so “General” is used if no category is provided. The README’s statement about the default is accurate—no changes required.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
README.md (2)
41-43: MAILTRAP_ACCOUNT_ID typo fixed consistently across config snippets.Confirmed the variable is now correct in all env blocks. Good catch and fix.
Also applies to: 63-65, 103-105, 222-224, 246-248, 268-270
142-143: Docs reflect “at least one of text/html” — ensure schema enforces it.The README now states mutual fallback between text/html. Please ensure sendEmail schema validates this (e.g., Zod refine).
🧹 Nitpick comments (2)
README.md (2)
7-8: Terminate the introductory sentence with a period.Minor clarity/style improvement.
-An MCP server that provides tools for sending transactional emails and managing email templates via Mailtrap +An MCP server that provides tools for sending transactional emails and managing email templates via Mailtrap.
117-130: Wrap example email addresses in backticks to avoid autolinking and satisfy MD lint.This addresses markdownlint MD034 and keeps examples clear.
-- "Send an email to john.doe@example.com with the subject 'Meeting Tomorrow' and a friendly reminder about our upcoming meeting." -- "Email sarah@example.com about the project update, and CC the team at team@example.com" +- "Send an email to `john.doe@example.com` with the subject 'Meeting Tomorrow' and a friendly reminder about our upcoming meeting." +- "Email `sarah@example.com` about the project update, and CC the team at `team@example.com`"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(9 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~121-~121: Consider using “email”.
Context: ...for example: Email Operations: - "Send an email to john.doe@example.com with the subject '...
(SEND_AN_EMAIL)
🪛 markdownlint-cli2 (0.17.2)
README.md
121-121: Bare URL used
(MD034, no-bare-urls)
122-122: Bare URL used
(MD034, no-bare-urls)
122-122: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (5)
README.md (5)
11-16: Clarify whether MAILTRAP_ACCOUNT_ID is required or optional.The client reportedly includes accountId conditionally from env. If optional, marking it as a prerequisite may mislead users. Suggest marking as optional and briefly explaining when it’s needed (e.g., organization-scoped accounts).
-4. Get your Account ID from [Mailtrap account management](https://mailtrap.io/account-management) +4. (Optional) Get your Account ID from [Mailtrap account management](https://mailtrap.io/account-management). If provided, API calls will be scoped to that account (recommended for organization accounts and template operations that require account scoping).
148-159: Confirm default category behavior matches implementation.Docs state category defaults to “General.” Verify the server sets this default (not just README). If not, either implement the default in code or remove the claim here.
160-167: LGTM: list-templates section is concise and clear.Docs accurately state no parameters required.
168-183: LGTM: update-template note prevents no-op requests.Nice addition to require at least one updatable field.
184-191: LGTM: delete-template parameters are clear.No issues.
…r dependency management
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
package.json(1 hunks)
… for dependency management
|
@VladimirTaytor @mklocek FYI, Ganna is testing its functionality https://railsware.slack.com/archives/C05GCAEA6CV/p1755335654013729?thread_ts=1754657674.208689&cid=C05GCAEA6CV |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
README.md (1)
141-142: Ensure schema enforces “at least one of text or html” (matches docs).Docs state one of text or html is required. Please verify the Zod schema for send-email actually enforces this with refine/superRefine.
#!/bin/bash # Verify sendEmail schema enforces at-least-one(text, html) set -euo pipefail echo "Looking for sendEmail schema definition and refine/superRefine usage..." ast-grep --pattern $'export default $_ = $_' 2>/dev/null | rg -nP 'sendEmailSchema' -n -C3 || true rg -nP --type=ts -C3 'sendEmailSchema' src || true rg -nP --type=ts -C3 '\.refine\s*\(' src/tools/sendEmail/schema.ts || true rg -nP --type=ts -C3 '\.superRefine\s*\(' src/tools/sendEmail/schema.ts || true rg -nP --type=ts -C3 '\b(text|html)\b' src/tools/sendEmail/schema.ts || true echo echo "If refine/superRefine is missing, add:" cat <<'ZOD' import { z } from "zod"; const sendEmailSchema = z.object({ from: z.string().email().optional(), to: z.string().email(), subject: z.string(), text: z.string().optional(), html: z.string().optional(), cc: z.array(z.string().email()).optional(), bcc: z.array(z.string().email()).optional(), category: z.string().optional(), }).refine(d => d.text || d.html, { message: "Either `text` or `html` must be provided", path: ["text", "html"], }); export default sendEmailSchema; ZOD
🧹 Nitpick comments (6)
CLAUDE.md (2)
38-44: Add a language identifier to the fenced code block (fixes MD040).markdownlint flags this block because it lacks a language. Use "text" to render the tree cleanly.
-``` +```text src/tools/{toolName}/ ├── index.ts # Main tool export ├── schema.ts # Zod validation schema ├── {toolName}.ts # Tool implementation └── __tests__/ # Jest test files -``` +```
46-50: Align “MAILTRAP_ACCOUNT_ID” optionality with README to avoid confusion.Here it’s documented as optional, but README’s Prerequisites currently list Account ID as a required step. Suggest updating README to mark it optional (only needed for multi‑account setups) for consistency across docs.
Would you like me to open a follow-up PR to harmonize this across README and examples?
README.md (4)
7-7: Add a period to complete the opening sentence.Minor polish.
-An MCP server that provides tools for sending transactional emails and managing email templates via Mailtrap +An MCP server that provides tools for sending transactional emails and managing email templates via Mailtrap.
40-42: Clarify that MAILTRAP_ACCOUNT_ID is optional in config examples.To prevent users thinking it’s mandatory, add a short note near these examples that MAILTRAP_ACCOUNT_ID can be omitted unless using multi‑account setups.
Proposed wording to add immediately below each example block:
- Note: MAILTRAP_ACCOUNT_ID is optional. Omit it if you don’t use multiple Mailtrap accounts.
Also applies to: 62-64, 101-104, 221-223, 245-247, 266-269
116-116: Fix article usage: “ask the agent”.Small grammar improvement.
-Once configured, you can ask agent to send emails and manage templates, for example: +Once configured, you can ask the agent to send emails and manage templates, for example:
9-16: Mark Account ID as optional & align withenginesfieldUpdate the prerequisites to call out that Account ID is only needed for multi-account setups and document the exact Node/Yarn versions per your
package.jsonengines.• File: README.md (lines 9–16)
• Change “Get your Account ID…” to optional
• Add “Node.js >=16.20.1” and “Yarn >=1.22.17” (tested on Node 20.x/24.x)## Prerequisites Before using this MCP server, you need to: 1. [Create a Mailtrap account](https://mailtrap.io/signup) 2. [Verify your domain](https://mailtrap.io/sending/domains) 3. Get your API token from [Mailtrap API settings](https://mailtrap.io/api-tokens) -4. Get your Account ID from [Mailtrap account management](https://mailtrap.io/account-management) +4. (Optional) Get your Account ID from [Mailtrap account management](https://mailtrap.io/account-management) — required only for multi-account setups. +5. Install Node.js >=16.20.1 and Yarn >=1.22.17 (LTS recommended). Tested on Node 20.x and Node 24.x.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
CLAUDE.md(1 hunks)README.md(9 hunks)package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~116-~116: There might be a mistake here.
Context: ...on. ## Usage Once configured, you can ask agent to send emails and manage templat...
(QB_NEW_EN)
[style] ~120-~120: Consider using “email”.
Context: ...for example: Email Operations: - "Send an email to john.doe@example.com with the subject '...
(SEND_AN_EMAIL)
[grammar] ~126-~126: There might be a mistake here.
Context: ...with subject 'Welcome to our platform!'" - "Update the template with ID 12345 to ch...
(QB_NEW_EN)
[grammar] ~180-~180: There might be a mistake here.
Context: ...New category for the template > [!NOTE] > At least one updatable field (name, su...
(QB_NEW_EN)
CLAUDE.md
[grammar] ~26-~26: There might be a mistake here.
Context: ... a modular pattern: ### Core Components - src/index.ts: Main MCP server entry po...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ...l tools and handles the server lifecycle - src/client.ts: Mailtrap client configu...
(QB_NEW_EN)
[grammar] ~28-~28: There might be a mistake here.
Context: ... client configuration and initialization - src/config/index.ts: Server configurat...
(QB_NEW_EN)
[grammar] ~31-~31: There might be a mistake here.
Context: ...uration constants ### Tool Architecture All tools follow a consistent pattern in...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ...t pattern in the src/tools/ directory: - Each tool has its own subdirectory (e.g....
(QB_NEW_EN)
[grammar] ~47-~47: There might be a mistake here.
Context: ...TOKEN: Required API token from Mailtrap - DEFAULT_FROM_EMAIL: Default sender email address - MAILTR...
(QB_NEW_EN)
[grammar] ~48-~48: There might be a mistake here.
Context: ...ROM_EMAIL: Default sender email address - MAILTRAP_ACCOUNT_ID`: Optional account ID for multi-account ...
(QB_NEW_EN)
[grammar] ~51-~51: There might be a mistake here.
Context: ... multi-account setups ### Testing Setup - Uses Jest with TypeScript support via ts...
(QB_NEW_EN)
[grammar] ~54-~54: There might be a mistake here.
Context: ... - Environment variables are set up via jest/setEnvVars.js - Coverage reports exclude test files and ...
(QB_NEW_EN)
[grammar] ~57-~57: There might be a mistake here.
Context: ...ype definitions ### Build Configuration - TypeScript compilation targets ES2022 wi...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
README.md
120-120: Bare URL used
(MD034, no-bare-urls)
121-121: Bare URL used
(MD034, no-bare-urls)
121-121: Bare URL used
(MD034, no-bare-urls)
CLAUDE.md
38-38: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (3)
CLAUDE.md (1)
62-69: Tool list looks comprehensive and clear.Nice, concise rundown of all five tools with consistent naming. Matches PR objectives.
README.md (2)
20-22: Quick-install link naming standardized.Good catch cleaning up to name=mailtrap consistently.
180-182: Good addition: note preventing no-op updates.Clear instruction that at least one updatable field is required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
AGENT.md (3)
3-10: Add template test commands and fix “single test” grammar; prefer npm test over raw jestReflect the new template test suites in the commands and fix the minor phrasing issue. Using
npm test --ensures the local Jest runner is used consistently.Apply this diff:
## Build/Lint/Test Commands - `npm run build` - Compile TypeScript to dist/ - `npm run lint` - Run ESLint and TypeScript checks - `npm test` - Run all Jest tests - `npm run test:watch` - Run tests in watch mode -- `jest src/tools/sendEmail/__tests__/sendEmail.test.ts` - Run single test file +- `npm test -- src/tools/sendEmail/__tests__/sendEmail.test.ts` - Run a single test file +- `npm test -- src/tools/templates/__tests__` - Run all template tool tests +- `npm test -- --verbose src/tools/templates/__tests__` - Run template tests with verbose output - `npm run dev` - Run MCP server with inspector for testing
12-17: Document newly registered template tools, fix wording, and note client accountId behaviorMake the architecture section match the current code: list all registered tools, fix minor grammar, and capture the conditional accountId logic in the client.
Apply this diff:
- MCP (Model Context Protocol) server integrating with Mailtrap email service + An MCP (Model Context Protocol) server that integrates with the Mailtrap email service - Main entry point: `src/index.ts` registers tools and handles server lifecycle -- Tools located in `src/tools/` with pattern: each tool has subdirectory with index.ts, schema.ts, implementation.ts, and __tests__/ +- Tools located in `src/tools/` with the pattern: each tool has a subdirectory with index.ts, schema.ts, implementation.ts, and __tests__/ +- Registered MCP tools: `send-email`, `create-template`, `list-templates`, `update-template`, `delete-template` +- Templates tooling: implementations in `src/tools/templates`, schemas in `src/tools/templates/schemas`, and aggregator exports in `src/tools/templates/index.ts` - Client configuration: `src/client.ts` handles Mailtrap API initialization +- Client behavior: conditionally includes `accountId` when `MAILTRAP_ACCOUNT_ID` is defined and is a valid number - Configuration: `src/config/index.ts` for server constants
17-18: Add Environment section for MAILTRAP variables (including MAILTRAP_ACCOUNT_ID)Explicitly document the environment variables used by the server and client.
Apply this diff:
- Configuration: `src/config/index.ts` for server constants + +## Environment +- `MAILTRAP_API_TOKEN` (required): API token used by the Mailtrap SDK. +- `MAILTRAP_ACCOUNT_ID` (optional): When set to a valid number, the client passes `accountId` to Mailtrap; otherwise it is omitted. ## Code Style
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
AGENT.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
AGENT.md
[grammar] ~8-~8: There might be a mistake here.
Context: ...endEmail/tests/sendEmail.test.ts- Run single test file -npm run dev` - Run ...
(QB_NEW_EN)
[grammar] ~11-~11: There might be a mistake here.
Context: ...h inspector for testing ## Architecture - MCP (Model Context Protocol) server inte...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ... integrating with Mailtrap email service - Main entry point: src/index.ts registe...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...ma.ts, implementation.ts, and tests/ - Client configuration: src/client.ts ha...
(QB_NEW_EN)
[grammar] ~18-~18: There might be a mistake here.
Context: ....ts` for server constants ## Code Style - Uses Airbnb TypeScript ESLint config wit...
(QB_NEW_EN)
Updated the documentation to specify that either `html` or `text` is required when creating an email template, improving clarity for users.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/tools/sendEmail/schema.ts (1)
5-26: Enforce “at least one of text or html” at the schema levelCurrently both
textandhtmlare optional with no cross-field validation. This allows invalid payloads that Mailtrap may reject at runtime. Prior review already requested this; echoing to keep it on the radar.Two safe options that won’t break current consumers if they import the default “fields object”:
- Option A (non-breaking): add a named export with the refined object schema while keeping the default export intact.
// Add this (in addition to the existing default export) export const sendEmailZodSchema = z .object(sendEmailSchema) .refine((d) => !!d.text || !!d.html, { message: "Either `text` or `html` must be provided", path: ["text", "html"], });
- Option B (breaking): change the default export to the refined object schema and update all consumers.
-const sendEmailSchema = { +const sendEmailSchema = z.object({ from: hasDefaultFromEmail ? z .string() .email() .optional() .describe("Email address of the sender (optional with default)") : z.string().email().describe("Email address of the sender"), to: z.string().email().describe("Email address of the recipient"), subject: z.string().describe("Email subject line"), cc: z.array(z.string().email()).optional().describe("Optional CC recipients"), bcc: z .array(z.string().email()) .optional() .describe("Optional BCC recipients"), category: z.string().describe("Email category for tracking"), text: z.string().optional().describe("Email body text"), html: z .string() .optional() .describe("Optional HTML version of the email body"), -}; +}).refine((d) => !!d.text || !!d.html, { + message: "Either `text` or `html` must be provided", + path: ["text", "html"], +});If you want, I can search the repo to see how
sendEmailSchemais consumed and propose the safest migration path.README.md (1)
141-146: Makecategoryrequired to match the schema and typesDocs currently say optional, but the schema requires it. This discrepancy will confuse users and break requests if omitted.
- `to` (required): Email address of the recipient - `subject` (required): Email subject line - `from` (optional): Email address of the sender, if not provided "DEFAULT_FROM_EMAIL" will be used - `text` (optional): Email body text, required if "html" is empty - `html` (optional): HTML version of the email body, required if "text" is empty - `cc` (optional): Array of CC recipient email addresses - `bcc` (optional): Array of BCC recipient email addresses -- `category` (optional): Email category for tracking +- `category` (required): Email category for tracking
🧹 Nitpick comments (3)
README.md (3)
116-116: Grammar: Add “the”Minor readability fix.
-Once configured, you can ask agent to send emails and manage templates, for example: +Once configured, you can ask the agent to send emails and manage templates, for example:
149-158: Verify whether “category defaults to ‘General’” is actually implementedDocs state a default category, but I don’t see evidence in this diff that server logic sets it. If not enforced in code, remove the default claim to avoid misleading users.
Option 1: If there is no code default, adjust docs:
-- `category` (optional): Template category (defaults to "General") +- `category` (optional): Template categoryOption 2: If you want this behavior, implement the default in the create-template handler and keep the doc as-is. I can draft a patch in the handler to set
category ??= "General".
13-16: Add an explicit Node.js version prerequisite (to address build issues on Node 22)There’s a report that compilation fails on Node v22. The README already uses Node 20.6.1 in examples; let’s set clear expectations to reduce setup friction.
## Prerequisites Before using this MCP server, you need to: +0. Use Node.js 20.x LTS (recommended). Newer majors (e.g., 22.x) may not be supported yet. 1. [Create a Mailtrap account](https://mailtrap.io/signup) 2. [Verify your domain](https://mailtrap.io/sending/domains) 3. Get your API token from [Mailtrap API settings](https://mailtrap.io/api-tokens) 4. Get your Account ID from [Mailtrap account management](https://mailtrap.io/account-management)If you prefer, we can instead set an engines field in package.json and document it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
README.md(9 hunks)src/tools/sendEmail/schema.ts(1 hunks)src/types/mailtrap.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/types/mailtrap.ts
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~116-~116: There might be a mistake here.
Context: ...on. ## Usage Once configured, you can ask agent to send emails and manage templat...
(QB_NEW_EN)
[style] ~120-~120: Consider using “email”.
Context: ...for example: Email Operations: - "Send an email to john.doe@example.com with the subject '...
(SEND_AN_EMAIL)
[grammar] ~126-~126: There might be a mistake here.
Context: ...with subject 'Welcome to our platform!'" - "Update the template with ID 12345 to ch...
(QB_NEW_EN)
[grammar] ~180-~180: There might be a mistake here.
Context: ...New category for the template > [!NOTE] > At least one updatable field (name, su...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
README.md
120-120: Bare URL used
(MD034, no-bare-urls)
121-121: Bare URL used
(MD034, no-bare-urls)
121-121: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (4)
README.md (4)
7-8: Nice concise scope statementClear positioning that the server handles transactional emails and template management.
20-23: Good: Quick-install links now use a consistent server name (“mailtrap”)This avoids duplicate server entries in clients.
40-42: LGTM: Environment variable name is correct (MAILTRAP_ACCOUNT_ID)This addresses the prior typo and prevents misconfiguration.
180-182: Good addition: prevents no-op update callsThe note clarifies that at least one field must be provided.
…or email tracking and analytics
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
README.md (3)
40-42: Env var typo fixed across examplesMAILTRAP_ACCOUNT_ID is correctly spelled and documented here.
62-64: Consistency: env section looks goodEnv keys match other sections and prior fixes.
141-146: Docs ↔ schema alignment for send-email (at-least-one text/html; category required)The README clearly states “text required if html is empty” and marks category as required. Please ensure the sendEmail Zod schema enforces the same via a refine and that category is required there as well.
If not already implemented, I can push a patch adding
.refine((d) => !!d.text || !!d.html, { message: "Either text or html must be provided", path: ["text","html"] }).
🧹 Nitpick comments (4)
README.md (4)
7-16: Clarify scope and add Node version to prerequisitesGood addition overall. Given recent reports (see PR comments) about build issues on Node 22.x, recommend documenting the supported Node.js version here to reduce setup friction. Also add terminal punctuation for the intro sentence.
- An MCP server that provides tools for sending transactional emails and managing email templates via Mailtrap + An MCP server that provides tools for sending transactional emails and managing email templates via Mailtrap. @@ 1. [Create a Mailtrap account](https://mailtrap.io/signup) 2. [Verify your domain](https://mailtrap.io/sending/domains) 3. Get your API token from [Mailtrap API settings](https://mailtrap.io/api-tokens) 4. Get your Account ID from [Mailtrap account management](https://mailtrap.io/account-management) +5. Install Node.js 20.x LTS (recommended). Example configs below use 20.6.1; newer major versions (e.g., 22.x) may require additional setup.Would you like me to add an “Engines” field to package.json in a follow-up PR so tooling warns early?
116-129: Tighten wording and avoid linter noise in examplesMinor grammar/polish and wrap emails/IDs to avoid MD034 complaints and improve readability.
-Once configured, you can ask agent to send emails and manage templates, for example: +Once configured, you can ask the agent to send emails and manage templates. For example: @@ -**Email Operations:** +**Email operations:** @@ -- "Send an email to john.doe@example.com with the subject 'Meeting Tomorrow' and a friendly reminder about our upcoming meeting." -- "Email sarah@example.com about the project update, and CC the team at team@example.com" +- "Send an email to <john.doe@example.com> with the subject 'Meeting Tomorrow' and a friendly reminder about our upcoming meeting." +- "Email <sarah@example.com> about the project update, and CC the team at <team@example.com>." @@ -**Template Operations:** +**Template operations:** @@ -- "List all email templates in my Mailtrap account" -- "Create a new email template called 'Welcome Email' with subject 'Welcome to our platform!'" -- "Update the template with ID 12345 to change the subject to 'Updated Welcome Message'" -- "Delete the template with ID 67890" +- "List all email templates in my Mailtrap account." +- "Create a new email template called 'Welcome Email' with the subject 'Welcome to our platform!'." +- "Update the template with ID `12345` to change the subject to 'Updated Welcome Message'." +- "Delete the template with ID `67890`."
153-158: Template create: confirm Mailtrap API requires category; clarify conditional html/text phrasingCurrent text says category is optional with default “General”. Mailtrap’s docs may require category; if your server sets a default internally, keep it optional but clarify it’s a server-side default. Also, match the send-email phrasing for html/text.
-- `html` (or `text` is required): HTML content of the template -- `text` (or `html` is required): Plain text version of the template -- `category` (optional): Template category (defaults to "General") +- `html` (optional): HTML content of the template (required if `text` is empty) +- `text` (optional): Plain text version of the template (required if `html` is empty) +- `category` (optional): Template category. If omitted, the server defaults it to "General".If the API actually mandates
category, change the last bullet to “(required)” and update the schema accordingly.
171-182: Good: “update-template” has a no-op guard; minor wording tweakSolid addition to prevent empty updates. Minor style improvement below.
-> [!NOTE] -> At least one updatable field (name, subject, html, text, or category) must be provided when calling update-template to perform an update. +> [!NOTE] +> Provide at least one of `name`, `subject`, `html`, `text`, or `category` when calling `update-template`; otherwise, the request is a no-op.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
README.md(9 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~116-~116: There might be a mistake here.
Context: ...on. ## Usage Once configured, you can ask agent to send emails and manage templat...
(QB_NEW_EN)
[style] ~120-~120: Consider using “email”.
Context: ...for example: Email Operations: - "Send an email to john.doe@example.com with the subject '...
(SEND_AN_EMAIL)
[grammar] ~126-~126: There might be a mistake here.
Context: ...with subject 'Welcome to our platform!'" - "Update the template with ID 12345 to ch...
(QB_NEW_EN)
[grammar] ~180-~180: There might be a mistake here.
Context: ...New category for the template > [!NOTE] > At least one updatable field (name, su...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
README.md
120-120: Bare URL used
(MD034, no-bare-urls)
121-121: Bare URL used
(MD034, no-bare-urls)
121-121: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (5)
README.md (5)
20-22: Nice: quick-install links are consistent and include MAILTRAP_ACCOUNT_IDThe Cursor/VS Code links now align on name=mailtrap and propagate the new env var. This prevents duplicate server entries and setup drift.
102-104: VS Code env sample OKKeys and wording consistent with earlier sections.
221-223: Env sample (Claude/Cursor dev config) consistentNo action needed.
245-247: Env sample (asdf + node path) consistentLooks good.
267-269: Env sample (VS Code dev config) consistentAll env keys align; no concerns.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/tools/sendEmail/__tests__/sendEmail.test.ts (6)
11-16: Category added to fixture is good; add a negative test to enforce the new requirementHaving
categoryinmockEmailDataaligns with the updated schema. To prevent regressions, add a test that omitscategoryand asserts an error result (useexpect.stringContaining("category")to avoid coupling to exact Zod message). You’ll need a type escape since the TS type now requirescategory.Add this test under the "errors handling" describe block:
it("should return error result when category is missing", async () => { const result = await sendEmail({ to: mockEmailData.to, subject: mockEmailData.subject, text: mockEmailData.text, // category omitted on purpose } as any); expect(client.send).not.toHaveBeenCalled(); expect(result).toEqual({ content: [ { type: "text", text: expect.stringContaining("category"), }, ], isError: true, }); });
31-38: Make expectation resilient: use objectContaining and drop html: undefinedIf the implementation ever omits undefined fields (e.g., drops
htmlentirely), this test becomes brittle. Preferexpect.objectContainingand excludehtmlwhen it’s undefined.Apply this diff:
- expect(client.send).toHaveBeenCalledWith({ - from: { email: "default@example.com" }, - to: [{ email: mockEmailData.to }], - subject: mockEmailData.subject, - text: mockEmailData.text, - html: undefined, - category: mockEmailData.category, - }); + expect(client.send).toHaveBeenCalledWith( + expect.objectContaining({ + from: { email: "default@example.com" }, + to: [{ email: mockEmailData.to }], + subject: mockEmailData.subject, + text: mockEmailData.text, + category: mockEmailData.category, + }) + );
58-65: Same resilience for the custom-from caseMirror the
objectContainingapproach and avoid assertinghtml: undefined.- expect(client.send).toHaveBeenCalledWith({ - from: { email: customFrom }, - to: [{ email: mockEmailData.to }], - subject: mockEmailData.subject, - text: mockEmailData.text, - html: undefined, - category: mockEmailData.category, - }); + expect(client.send).toHaveBeenCalledWith( + expect.objectContaining({ + from: { email: customFrom }, + to: [{ email: mockEmailData.to }], + subject: mockEmailData.subject, + text: mockEmailData.text, + category: mockEmailData.category, + }) + );
87-96: Same resilience for CC/BCC caseUse
objectContainingand avoidhtml: undefinedto reduce brittleness while still asserting the essentials.- expect(client.send).toHaveBeenCalledWith({ - from: { email: "default@example.com" }, - to: [{ email: mockEmailData.to }], - subject: mockEmailData.subject, - text: mockEmailData.text, - html: undefined, - category: mockEmailData.category, - cc: cc.map((email) => ({ email })), - bcc: bcc.map((email) => ({ email })), - }); + expect(client.send).toHaveBeenCalledWith( + expect.objectContaining({ + from: { email: "default@example.com" }, + to: [{ email: mockEmailData.to }], + subject: mockEmailData.subject, + text: mockEmailData.text, + category: mockEmailData.category, + cc: cc.map((email) => ({ email })), + bcc: bcc.map((email) => ({ email })), + }) + );
116-123: Keep asserting html when provided, but still wrap with objectContainingHere,
htmlis present. UseobjectContainingfor forward-compatibility.- expect(client.send).toHaveBeenCalledWith({ - from: { email: "default@example.com" }, - to: [{ email: mockEmailData.to }], - subject: mockEmailData.subject, - text: mockEmailData.text, - html, - category: mockEmailData.category, - }); + expect(client.send).toHaveBeenCalledWith( + expect.objectContaining({ + from: { email: "default@example.com" }, + to: [{ email: mockEmailData.to }], + subject: mockEmailData.subject, + text: mockEmailData.text, + html, + category: mockEmailData.category, + }) + );
162-169: Rename test to reflect behavior (function returns an error result, it doesn’t throw)The test title says “throw” but the assertion expects an error result object. Rename for clarity. Adding
categoryto the payload here is correct to isolate the intended validation branch.- it("should throw error when neither HTML nor TEXT is provided", async () => { + it("should return an error result when neither HTML nor TEXT is provided", async () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/tools/sendEmail/__tests__/sendEmail.test.ts(6 hunks)
Motivation
The Mailtrap MCP server needed comprehensive unit test coverage for all template management tools to ensure code quality, reliability, and maintainability. The existing codebase only had tests for the
sendEmailtool, but the template tools (createTemplate,deleteTemplate,listTemplates,updateTemplate) lacked proper test coverage.Changes
sendEmail.test.tsHow to test
npm run buildto ensure TypeScript compilation succeedsnpm test -- src/tools/templates/__tests__to verify all 31 tests passnpm test -- --verbose src/tools/templates/__tests__to see detailed test outputSummary by CodeRabbit
New Features
Documentation
Tests
Chores