feat(notifications): add webhook:// notification target - #1623
Open
s0up4200 wants to merge 4 commits into
Open
Conversation
Sends a flat JSON payload with source_app: "qui" to a configurable HTTP endpoint. Reuses existing notifiarrapi sub-object builders for torrent, backup, cross-seed, dir scan, orphan scan, and automation event data.
Contributor
WalkthroughThe changes introduce webhook notification support to the notification service. The implementation includes URL validation for webhook schemes, payload construction from event data, and HTTP POST delivery to configured webhook endpoints. Service routing and dispatch logic are updated to handle webhooks alongside existing notification channels. Changes
Sequence DiagramsequenceDiagram
participant Service
participant PayloadBuilder
participant URLValidator
participant WebhookTarget
Service->>URLValidator: parseWebhookURL(config)
URLValidator-->>Service: validated URL
Service->>PayloadBuilder: buildWebhookPayload(event data)
PayloadBuilder->>PayloadBuilder: extract event fields<br/>(Torrent, CrossSeed, etc.)
PayloadBuilder-->>Service: webhookPayload struct
Service->>Service: Marshal payload to JSON
Service->>WebhookTarget: POST request<br/>(headers, body, timeout)
WebhookTarget-->>Service: HTTP response
alt Status 2xx
Service->>Service: Drain response body
else Status non-2xx
Service->>Service: Error on status
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/services/notifications/webhook_test.go (1)
17-277: Great coverage; consider consolidating repeated scenarios into table-driven tests.The test surface is strong, but several webhook payload/send cases repeat setup/assert patterns and would be easier to maintain as table-driven cases.
As per coding guidelines, "Use table-driven test cases in Go tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/notifications/webhook_test.go` around lines 17 - 277, Tests repeat similar setups and assertions; refactor into table-driven tests to improve maintainability by consolidating scenarios for parseWebhookURL, sendWebhook, buildWebhookPayload and ValidateURL. Create parameterized test tables (slices of structs) for TestParseWebhookURL, TestSendWebhookSuccess/TestSendWebhookHTTPError/TestSendWebhookNoAuthHeaders, and the various buildWebhookPayload cases that vary only by Event fields and expected payload subobjects; iterate the table in a single t.Run subtest, reuse common server/Service setup (Service{} and httptest.Server) and common assertions (status, headers, payload fields) while keeping unique expectations per case, and ensure each case still calls parseWebhookURL, svc.sendWebhook, svc.buildWebhookPayload or ValidateURL as appropriate. Use the existing identifiers (parseWebhookURL, Service.sendWebhook, buildWebhookPayload, ValidateURL, and test names) so the refactor replaces duplicated code with table entries and per-case assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/services/notifications/webhook.go`:
- Around line 85-87: The non-2xx response path currently ignores the error from
io.ReadAll; update the error handling in the webhook send routine so the call to
io.ReadAll(io.LimitReader(res.Body, 4096)) checks its returned error and
includes that error (and any successfully read body chunk) in the returned
fmt.Errorf message; in other words, capture (body, err) from io.ReadAll, and if
err != nil return or wrap fmt.Errorf("unexpected status: %d body-read-error: %v
partial-body: %s", res.StatusCode, err, strings.TrimSpace(string(body)))
otherwise return the existing unexpected-status error using the trimmed body.
Reference res.StatusCode, res.Body, io.ReadAll, io.LimitReader and
strings.TrimSpace when locating the change.
---
Nitpick comments:
In `@internal/services/notifications/webhook_test.go`:
- Around line 17-277: Tests repeat similar setups and assertions; refactor into
table-driven tests to improve maintainability by consolidating scenarios for
parseWebhookURL, sendWebhook, buildWebhookPayload and ValidateURL. Create
parameterized test tables (slices of structs) for TestParseWebhookURL,
TestSendWebhookSuccess/TestSendWebhookHTTPError/TestSendWebhookNoAuthHeaders,
and the various buildWebhookPayload cases that vary only by Event fields and
expected payload subobjects; iterate the table in a single t.Run subtest, reuse
common server/Service setup (Service{} and httptest.Server) and common
assertions (status, headers, payload fields) while keeping unique expectations
per case, and ensure each case still calls parseWebhookURL, svc.sendWebhook,
svc.buildWebhookPayload or ValidateURL as appropriate. Use the existing
identifiers (parseWebhookURL, Service.sendWebhook, buildWebhookPayload,
ValidateURL, and test names) so the refactor replaces duplicated code with table
entries and per-case assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 736703cd-b37f-4be7-9b0c-ac685068dc5a
📒 Files selected for processing (3)
internal/services/notifications/service.gointernal/services/notifications/webhook.gointernal/services/notifications/webhook_test.go
Comment on lines
+85
to
+87
| if res.StatusCode < 200 || res.StatusCode >= 300 { | ||
| body, _ := io.ReadAll(io.LimitReader(res.Body, 4096)) | ||
| return fmt.Errorf("unexpected status: %d body: %s", res.StatusCode, strings.TrimSpace(string(body))) |
Contributor
There was a problem hiding this comment.
Handle response body read errors explicitly on non-2xx responses.
io.ReadAll errors are currently ignored, which can hide useful failure context when diagnosing webhook delivery issues.
💡 Suggested fix
if res.StatusCode < 200 || res.StatusCode >= 300 {
- body, _ := io.ReadAll(io.LimitReader(res.Body, 4096))
- return fmt.Errorf("unexpected status: %d body: %s", res.StatusCode, strings.TrimSpace(string(body)))
+ body, readErr := io.ReadAll(io.LimitReader(res.Body, 4096))
+ if readErr != nil {
+ return fmt.Errorf("unexpected status: %d (failed to read response body: %w)", res.StatusCode, readErr)
+ }
+ return fmt.Errorf("unexpected status: %d body: %s", res.StatusCode, strings.TrimSpace(string(body)))
}As per coding guidelines, "Prefer explicit error handling over silent failures in Go".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/services/notifications/webhook.go` around lines 85 - 87, The non-2xx
response path currently ignores the error from io.ReadAll; update the error
handling in the webhook send routine so the call to
io.ReadAll(io.LimitReader(res.Body, 4096)) checks its returned error and
includes that error (and any successfully read body chunk) in the returned
fmt.Errorf message; in other words, capture (body, err) from io.ReadAll, and if
err != nil return or wrap fmt.Errorf("unexpected status: %d body-read-error: %v
partial-body: %s", res.StatusCode, err, strings.TrimSpace(string(body)))
otherwise return the existing unexpected-status error using the trimmed body.
Reference res.StatusCode, res.Body, io.ReadAll, io.LimitReader and
strings.TrimSpace when locating the change.
8 tasks
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.
Sends a flat JSON payload with source_app: "qui" to a configurable HTTP endpoint. Reuses existing notifiarrapi sub-object builders for torrent, backup, cross-seed, dir scan, orphan scan, and automation event data.
Summary by CodeRabbit