Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds a new Email Logs API to the Mailtrap Node.js client: types, API resource (list/get), a MailtrapClient getter, tests, example usage, and documentation/changelog updates. Includes deepObject query serialization for filters and pagination support. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as MailtrapClient
participant Wrapper as EmailLogsBaseAPI
participant Resource as EmailLogsApi
participant Server as Mailtrap API
Client->>Wrapper: client.emailLogs.getList(params)
Wrapper->>Resource: bound getList(params)
Resource->>Server: GET /accounts/{id}/email_logs?deepObject...
Server-->>Resource: 200 OK (messages, next_page_cursor)
Resource-->>Wrapper: parsed response
Wrapper-->>Client: return EmailLogsList
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
examples/email-logs/everything.ts (1)
3-8: Consider usingparseIntwith validation for the placeholder.
Number("<YOUR-ACCOUNT-ID-HERE>")returnsNaN. While this is just an example, users copying this code might not realize the issue immediately. Consider adding a comment or validation.♻️ Suggested improvement
const TOKEN = "<YOUR-TOKEN-HERE>"; -const ACCOUNT_ID = "<YOUR-ACCOUNT-ID-HERE>"; +const ACCOUNT_ID = "<YOUR-ACCOUNT-ID-HERE>"; // Replace with your numeric account ID const client = new MailtrapClient({ token: TOKEN, - accountId: Number(ACCOUNT_ID), + accountId: parseInt(ACCOUNT_ID, 10), // Ensure this is a valid number });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/email-logs/everything.ts` around lines 3 - 8, The example currently uses Number(ACCOUNT_ID) which yields NaN for the placeholder; update the MailtrapClient instantiation to parse and validate the account ID (use parseInt(ACCOUNT_ID, 10) or Number with a check), then handle invalid values (throw a clear error or provide a helpful comment/instruction) before passing it into new MailtrapClient so TOKEN, ACCOUNT_ID and the MailtrapClient(...) call always receive a valid numeric accountId.src/__tests__/lib/mailtrap-client.test.ts (1)
915-916: Consider using theACCOUNT_ID_MISSINGconstant for consistency.Other getter tests (e.g., lines 724, 761, 787) use
new MailtrapError(ACCOUNT_ID_MISSING)for assertions, but this test uses a hardcoded string. Using the constant improves maintainability if the message changes again.♻️ Suggested refactor
expect(() => client.sendingDomains).toThrow( - "accountId is missing, please provide a valid accountId." + ACCOUNT_ID_MISSING );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/lib/mailtrap-client.test.ts` around lines 915 - 916, The test for client.sendingDomains currently asserts a hardcoded error string; update it to assert the canonical error object using the ACCOUNT_ID_MISSING constant (i.e., replace the string-based toThrow assertion with toThrow(new MailtrapError(ACCOUNT_ID_MISSING))) so it matches other getter tests that use MailtrapError and ACCOUNT_ID_MISSING for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/lib/api/resources/EmailLogs.test.ts`:
- Around line 71-81: The response interceptor added in the beforeAll (via
axios.interceptors.response.use with handleSendingError) is never removed;
capture the interceptor id when calling use() (e.g., store the return value from
axios.interceptors.response.use in a variable) and add an afterAll that ejects
it with axios.interceptors.response.eject(interceptorId); also ensure any
AxiosMockAdapter cleanup (mock.reset/restore) remains in afterEach/afterAll as
appropriate so the global axios instance is restored for other tests.
---
Nitpick comments:
In `@examples/email-logs/everything.ts`:
- Around line 3-8: The example currently uses Number(ACCOUNT_ID) which yields
NaN for the placeholder; update the MailtrapClient instantiation to parse and
validate the account ID (use parseInt(ACCOUNT_ID, 10) or Number with a check),
then handle invalid values (throw a clear error or provide a helpful
comment/instruction) before passing it into new MailtrapClient so TOKEN,
ACCOUNT_ID and the MailtrapClient(...) call always receive a valid numeric
accountId.
In `@src/__tests__/lib/mailtrap-client.test.ts`:
- Around line 915-916: The test for client.sendingDomains currently asserts a
hardcoded error string; update it to assert the canonical error object using the
ACCOUNT_ID_MISSING constant (i.e., replace the string-based toThrow assertion
with toThrow(new MailtrapError(ACCOUNT_ID_MISSING))) so it matches other getter
tests that use MailtrapError and ACCOUNT_ID_MISSING for consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12dc955b-e82f-4d66-af74-2e08dec5ef24
📒 Files selected for processing (10)
CHANGELOG.mdREADME.mdexamples/email-logs/everything.tssrc/__tests__/lib/api/resources/EmailLogs.test.tssrc/__tests__/lib/mailtrap-client.test.tssrc/config/index.tssrc/lib/MailtrapClient.tssrc/lib/api/EmailLogs.tssrc/lib/api/resources/EmailLogs.tssrc/types/api/email-logs.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Add support for Email Logs API (https://docs.mailtrap.io/developers/email-sending/email-logs).
Summary by CodeRabbit
New Features
Documentation
Tests
Bug Fixes