Conversation
WalkthroughThe changes restructure the Attachment download API to return a result object containing Data and Headers instead of raw bytes, update multiple webhook-related types to use nullable pointers for improved nil-safety semantics, and add defensive nil-checks to webhook helper methods. Changes
Sequence DiagramsequenceDiagram
participant Client
participant AttachmentService
participant HTTP
rect rgb(200, 220, 240)
Note over AttachmentService: Old Flow: []byte return
Client->>AttachmentService: Download(ctx, emailID, filename)
AttachmentService->>HTTP: GET /attachment
HTTP-->>AttachmentService: Response + Body
AttachmentService-->>Client: ([]byte, error)
end
rect rgb(240, 220, 200)
Note over AttachmentService: New Flow: Structured Response
Client->>AttachmentService: Download(ctx, emailID, filename)
AttachmentService->>HTTP: GET /attachment
HTTP-->>AttachmentService: Response + Body
AttachmentService->>AttachmentService: Extract Data & Headers
AttachmentService-->>Client: (AttachmentDownloadResponse{Data, Headers}, error)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes The changes span multiple file categories with varying complexity: the attachment API restructuring is straightforward (~15 min), but the extensive webhook type field conversions to nullable pointers require careful semantic review to verify nil-safety and backward compatibility across multiple interdependent types (~30 min), plus webhook helper method nil-checks and test updates (~5–10 min). Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![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)
attachment_test.go (1)
124-134: Good validations on data and headers; add a couple more assertions
- Also assert Content-Disposition matches filename.
- Assert server received no Content-Type header for GET download to enforce the contract.
Example additions inside the httptest handler and assertions:
// inside handler, before sending response: if ct := r.Header.Get("Content-Type"); ct != "" { t.Errorf("Expected no Content-Type on GET download, got '%s'", ct) } // after result.Headers checks: if cd := result.Headers.Get("Content-Disposition"); cd == "" || !strings.Contains(cd, tt.filename) { t.Errorf("Expected Content-Disposition to include filename '%s', got '%s'", tt.filename, cd) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
attachment_test.go(3 hunks)inbound.go(4 hunks)types.go(5 hunks)webhook.go(1 hunks)webhook_test.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Use PascalCase for exported types, methods, and functions; use camelCase for unexported identifiers
All exported functions must have GoDoc comments
Keep comments concise and focused on usage, not implementation details
Files:
inbound.gowebhook_test.gowebhook.gotypes.goattachment_test.go
inbound.go
📄 CodeRabbit inference engine (AGENTS.md)
inbound.go: Include an API Reference: link in GoDoc comments for exported API methods where applicable
Return Go errors for network/client failures; return API errors via ApiResponse.Error
Use fmt.Errorf with %w for error wrapping
For direct data returns (e.g., attachment Download), return standard Go errors (not ApiResponse)
Provide pointer helper functions: String, Int, Bool at the bottom of the file
When adding a new API method, implement the method on the appropriate service in inbound.go
When adding a new API method, include a GoDoc comment with an API reference link
When adding a new service, define the service struct holding a *Inbound client reference
When adding a new service, add a constructor: func NewX(client *Inbound) *X
When adding a new service, add an accessor on Inbound: func (c *Inbound) X() *X
Implement service methods using the generic makeRequest[T] helper
All API methods should return *ApiResponse[T] for typed responses
Use buildQueryString() to construct GET query parameters from request structs
Escape URL path parameters with url.PathEscape()
HTTP client default timeout is 30s; allow customization via WithHTTPClient()
Include Authorization: Bearer header on all requests
Set Content-Type: application/json on all requests except attachment downloads
All API methods accept context.Context as the first parameter and use http.NewRequestWithContext
Respect context timeouts and cancellations in request execution
Support idempotency for send, reply, and schedule operations using IdempotencyOptions and the Idempotency-Key header
Follow the provided method template: take ctx, build endpoint (with buildQueryString for GET), and call makeRequest[T]
For optional headers (e.g., Idempotency-Key), build a headers map conditionally and pass to makeRequest
Files:
inbound.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Organize tests as one test file per service (e.g., send_email_test.go, webhook_test.go)
Use httptest.NewServer to mock API responses in tests
Test both success and error cases for service methods
Verify HTTP method, Authorization header, and request body in tests
Structure tests as table-driven tests with t.Run subtests
Files:
webhook_test.goattachment_test.go
types.go
📄 CodeRabbit inference engine (AGENTS.md)
types.go: Prefix request types with the HTTP method (e.g., PostEmailsRequest, GetMailRequest)
Suffix response types with Response (e.g., PostEmailsResponse)
Use pointers for optional fields (e.g., *string, *int, *bool)
Use type any for fields that can accept multiple types (e.g., string or []string)
Add JSON tags to all struct fields
Use omitempty for optional request fields in JSON tags
When adding a new API method, define request/response types in types.go
Ensure query parameters are driven by struct fields with json tags
Files:
types.go
🧠 Learnings (1)
📚 Learning: 2025-10-09T20:15:43.730Z
Learnt from: CR
PR: inboundemail/inbound-golang-sdk#0
File: AGENTS.md:0-0
Timestamp: 2025-10-09T20:15:43.730Z
Learning: Applies to inbound.go : Provide pointer helper functions: String, Int, Bool at the bottom of the file
Applied to files:
inbound.go
🧬 Code graph analysis (2)
inbound.go (1)
types.go (1)
AttachmentDownloadResponse(803-806)
webhook.go (1)
types.go (1)
WebhookPayload(728-733)
⏰ 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). (3)
- GitHub Check: test (1.22.x, windows-latest)
- GitHub Check: test (1.21.x, windows-latest)
- GitHub Check: test (1.21.x, macos-latest)
🔇 Additional comments (20)
inbound.go (3)
303-323: Docs/idempotency for Send look goodAPI reference link present; conditional Idempotency-Key header applied only when provided.
347-359: Schedule docs and header handling LGTMClear usage notes and proper optional idempotency header wiring.
754-767: Pointer helpers provided as requiredString/Int/Bool helpers present at file bottom.
Based on learnings
attachment_test.go (1)
106-107: Test update to new return type looks correctStoring result and checking errors aligns with new signature.
types.go (9)
3-6: Imports for http/time are appropriateNeeded for http.Header in AttachmentDownloadResponse and time fields elsewhere.
589-599: ThreadSummary fields and tags consistentMatches existing naming conventions; optional LatestMessage pointer makes sense.
626-631: ThreadAttachment required fields OKKeeping these non-pointer aligns with thread retrieval guarantees.
729-745: WebhookPayload/WebhookEmailData pointer semantics align with nil-safetyEvent/timestamp remain required; From/To/Subject as pointers is consistent with incoming variability.
753-755: *WebhookAddress.Address as string prevents nil deref issuesMatches helper changes in webhook.go.
758-774: ParsedData optional fields moved to pointers; goodHeaders stays map[string]any; Priority as any covers string|false cases.
776-783: CleanedContent text/html as pointers is reasonableKeeps parity with parser variability.
785-792: WebhookAttachment pointer fields + non-pointer DownloadUrlThis mirrors JS SDK semantics; looks good.
802-806: AttachmentDownloadResponse shape looks rightData []byte + Headers http.Header matches new Download API.
webhook.go (2)
22-31: Nil-safety and formatting in GetFromAddress are solidDefensive checks on From and Address pointers prevent panics; formatted "Name " output is correct.
37-46: Mirrored improvements for GetToAddress LGTMSame robust handling for To; consistent behavior.
webhook_test.go (5)
114-129: Nil-aware checks for MessageID/Subject are correctClear error messages when nil vs wrong value.
146-160: ParsedData text/html assertions properly handle pointersGood split between presence and content checks.
163-177: CleanedContent text/html pointer checks LGTMMatches updated types.
179-185: HasHTML/HasText boolean checks preservedStraightforward and correct.
203-213: Endpoint field assertions intactIDs and types verified as expected.
| func (s *AttachmentService) Download(ctx context.Context, emailID, filename string) (*AttachmentDownloadResponse, error) { | ||
| endpoint := fmt.Sprintf("/attachments/%s/%s", emailID, url.PathEscape(filename)) | ||
|
|
||
| resp, err := s.client.request(ctx, "GET", endpoint, nil, nil) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| data, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if resp.StatusCode >= 400 { | ||
| respBody, _ := io.ReadAll(resp.Body) | ||
| var errorResp struct { | ||
| Error string `json:"error"` | ||
| } | ||
| if json.Unmarshal(respBody, &errorResp) == nil && errorResp.Error != "" { | ||
| return nil, fmt.Errorf("%s", errorResp.Error) | ||
| } | ||
| return nil, fmt.Errorf("HTTP %d: %s", resp.StatusCode, resp.Status) | ||
| } | ||
|
|
||
| return io.ReadAll(resp.Body) | ||
| return &AttachmentDownloadResponse{ | ||
| Data: data, | ||
| Headers: resp.Header, | ||
| }, nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Attachment download: escape all path params, avoid JSON Content-Type on binary GET, and check status before reading body
- Escape emailID too to prevent path issues.
- request() currently sets Content-Type: application/json on all requests; downloads should not send it.
- Return early on non-2xx before reading the body; also drain body on error for connection reuse.
Apply this diff inside Download:
- endpoint := fmt.Sprintf("/attachments/%s/%s", emailID, url.PathEscape(filename))
+ endpoint := fmt.Sprintf("/attachments/%s/%s", url.PathEscape(emailID), url.PathEscape(filename))
@@
- data, err := io.ReadAll(resp.Body)
- if err != nil {
- return nil, err
- }
-
- if resp.StatusCode >= 400 {
- return nil, fmt.Errorf("HTTP %d: %s", resp.StatusCode, resp.Status)
- }
+ if resp.StatusCode >= 400 {
+ // drain to allow connection reuse
+ _, _ = io.Copy(io.Discard, resp.Body)
+ return nil, fmt.Errorf("HTTP %d: %s", resp.StatusCode, resp.Status)
+ }
+ data, err := io.ReadAll(resp.Body)
+ if err != nil {
+ return nil, err
+ }Additionally, update request() to set Content-Type only when sending JSON:
// in func (c *Inbound) request(...)
req.Header.Set("Authorization", "Bearer "+c.apiKey)
// Set JSON Content-Type only when we actually have a JSON body
if body != nil {
req.Header.Set("Content-Type", "application/json")
}If you prefer not to touch request(), pass a header override in Download and then delete it before send; but adjusting request() is cleaner and aligns with “Set Content-Type: application/json on all requests except attachment downloads”. As per coding guidelines.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes