Add attachment download functionality#2
Conversation
- Add AttachmentService with Download method for downloading email attachments - Add DownloadUrl field to WebhookAttachment type - Add comprehensive tests for attachment downloads - Add AGENTS.md with project documentation for AI assistants Amp-Thread-ID: https://ampcode.com/threads/T-01712ca1-af4c-4679-983b-98421810bb50 Co-authored-by: Amp <amp@ampcode.com>
WalkthroughAdds AttachmentService with Download(ctx, emailID, filename) exposed via Inbound.Attachment(), adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Caller
participant SDK as Inbound (SDK)
participant AS as AttachmentService
participant HTTP as HTTP Server
Dev->>SDK: Attachment().Download(ctx, emailID, filename)
SDK->>AS: resolve service
AS->>HTTP: GET /attachments/{emailID}/{filename}\nAuthorization: Bearer <api-key>
alt 2xx
HTTP-->>AS: binary payload
AS-->>SDK: []byte
SDK-->>Dev: []byte
else 4xx/5xx
HTTP-->>AS: error status (+ optional JSON body)
AS-->>SDK: error
SDK-->>Dev: error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)attachment_test.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (4)
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 |
![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 (1)
AGENTS.md (1)
15-24: Add language identifier to fenced code block.The project structure code block on line 15 is missing a language identifier. While not critical for a directory tree, adding
textorplaintextas the identifier improves rendering consistency.Apply this diff:
-``` +```text . ├── inbound.go # Main client and service implementations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
AGENTS.md(1 hunks)attachment_test.go(1 hunks)inbound.go(3 hunks)types.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
attachment_test.go (2)
inbound.go (1)
NewClient(56-71)types.go (1)
WebhookAttachment(781-787)
🪛 markdownlint-cli2 (0.18.1)
AGENTS.md
9-9: Bare URL used
(MD034, no-bare-urls)
10-10: Bare URL used
(MD034, no-bare-urls)
15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
192-192: Bare URL used
(MD034, no-bare-urls)
193-193: Bare URL used
(MD034, no-bare-urls)
194-194: Bare URL used
(MD034, no-bare-urls)
195-195: Bare URL used
(MD034, no-bare-urls)
196-196: Bare URL used
(MD034, no-bare-urls)
197-197: Bare URL used
(MD034, no-bare-urls)
198-198: Bare URL used
(MD034, no-bare-urls)
199-199: Bare URL used
(MD034, no-bare-urls)
200-200: Bare URL used
(MD034, no-bare-urls)
201-201: Bare URL used
(MD034, no-bare-urls)
202-202: Bare URL used
(MD034, no-bare-urls)
203-203: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (1.22.x, windows-latest)
- GitHub Check: test (1.21.x, windows-latest)
🔇 Additional comments (6)
AGENTS.md (1)
1-260: LGTM! Comprehensive documentation for AI assistants.The documentation provides excellent guidance covering:
- Project structure and architecture
- Service patterns and conventions
- Testing guidelines with concrete examples
- API reference links
- Common implementation patterns
This will be very helpful for AI assistants working on the SDK.
inbound.go (3)
615-624: LGTM! Service structure follows established patterns.The
AttachmentServiceimplementation correctly follows the service pattern used throughout the SDK with a client reference and constructor.
672-674: LGTM! Accessor method correctly exposes the service.The
Attachment()accessor follows the established pattern for exposing services from the main client.
625-649: Fix unsafe error string formatting and clarify webhook download flow.The implementation has two concerns:
Line 643: Using
fmt.Errorf("%s", errorResp.Error)with an untrusted error string is unsafe if it contains format verbs like%sor%d. These would be interpreted as format specifiers, potentially causing panics or unexpected output.Webhook downloads: The method always constructs the endpoint from
emailIDandfilename, but doesn't utilizeWebhookAttachment.DownloadUrl. Please clarify: should this method support downloading directly from webhook-provided URLs, or should callers parse theDownloadUrlto extract parameters?Apply this diff to fix the error formatting:
if json.Unmarshal(respBody, &errorResp) == nil && errorResp.Error != "" { - return nil, fmt.Errorf("%s", errorResp.Error) + return nil, errors.New(errorResp.Error) }Add this import if not already present:
import ( "bytes" "context" "encoding/json" + "errors" "fmt"Likely an incorrect or invalid review comment.
attachment_test.go (1)
11-114: LGTM! Comprehensive test coverage.The test thoroughly covers:
- Successful downloads with standard and space-containing filenames
- Error scenarios (404, 401)
- HTTP method and authorization header verification
- Data integrity validation
types.go (1)
786-786: Clarify usage of DownloadUrl. The newDownloadUrlfield onWebhookAttachment(types.go:786) is set in tests but never used byAttachmentService.Download, which still constructs the URL fromemailID+filename. ShouldDownloadsupport a full URL, callers parse this field, or is it purely informational?
Amp-Thread-ID: https://ampcode.com/threads/T-01712ca1-af4c-4679-983b-98421810bb50 Co-authored-by: Amp <amp@ampcode.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
attachment_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
attachment_test.go (1)
inbound.go (1)
NewClient(56-71)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (1.22.x, windows-latest)
- GitHub Check: test (1.21.x, windows-latest)
🔇 Additional comments (1)
attachment_test.go (1)
30-36: Verify URL encoding for filename with spaces.With the URL path validation added (see previous comment), this test case will properly verify that spaces in filenames are correctly encoded as
%20in the URL path.Once you implement the URL path validation, this test case will automatically validate proper encoding of the space character.
- Add errorContains field validation to check error messages - Validate URL path construction and filename escaping - Add net/url import for PathEscape validation
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
attachment_test.go (1)
13-129: Consider adding edge case tests.While the current test coverage is solid, consider adding tests for:
- Empty
emailIDorfilenameparameters to ensure proper validation- Network timeout scenarios if the
Downloadmethod handles them- Server returning unexpected content types
These would strengthen the test suite but are not critical for the initial implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
attachment_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
attachment_test.go (1)
inbound.go (1)
NewClient(56-71)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (1.22.x, windows-latest)
- GitHub Check: test (1.21.x, windows-latest)
🔇 Additional comments (5)
attachment_test.go (5)
1-11: LGTM! Imports are appropriate.All necessary packages are imported and used in the tests.
13-63: LGTM! Test cases cover key scenarios.The test table includes appropriate success cases (including filename with spaces) and error cases. The
errorContainsfield is properly defined and matches the mock server's error responses.
67-98: LGTM! Mock server properly validates requests.The handler correctly validates the HTTP method, URL path with proper escaping (addressing previous review feedback), and Authorization header. Response handling for both success and error cases is appropriate.
101-104: LGTM! Client setup follows standard test pattern.Client creation is straightforward with appropriate error handling.
119-126: LGTM! Success case assertions are correct.Proper verification that no error occurred and the downloaded data matches the expected response.
1 participant
Summary by CodeRabbit
New Features
Documentation
Types
Tests