fix(notifications): improve torrent and automation event samples

[s0up4200]

Summary

• delay torrent_added notifications by 10 seconds and refresh torrent stats before dispatch
• keep notifiarrapi torrent metric fields present for torrent events even when values are zero
• include torrent metric lines in formatted torrent notifications for shoutrrr/discord rendering
• expand automation summary samples beyond delete-only actions when torrent names are available
• add regression coverage for delayed refresh/fallback behavior and zero-value payload fields

Validated with go test ./cmd/qui/... ./internal/services/notifications/... ./internal/services/automations/... and earlier full make test/make build in this branch context.

Summary by CodeRabbit

• New Features

  • Notifications include comprehensive torrent metrics with optional human-readable formatting.

• Improvements

  • Added a brief delay for torrent-added notifications to refresh snapshots before sending.
  • Tracker domain and tag extraction moved to centralized helpers.
  • Automation summaries now surface deduplicated, limited torrent name samples across more actions.

• Tests

  • Added tests for notification formatting, delayed notification behavior, and sample collection.

• Documentation

  • Added owner collaboration notes for review workflow.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Refactors torrent notification handling: centralizes event builders, adds delayed refresh-before-send logic for added-torrent notifications, encapsulates tracker/tag parsing, expands automation sampling to include torrent context, and gates/injects torrent metrics with human-readable formatting in notifications.

Changes

Cohort / File(s)	Summary
CLI notification wiring cmd/qui/main.go	Replaced inline torrent event construction with centralized helpers (buildTorrentCompletedEvent, notifyTorrentAddedWithDelay).
Torrent notification helpers cmd/qui/torrent_notifications.go	New helpers and torrentNotificationSync interface: build completed/added events, parse tags, derive tracker domain, schedule delayed notify with pre-send refresh and timeout (torrentAddedNotificationDelay, torrentAddedRefreshTimeout).
Torrent notification tests cmd/qui/torrent_notifications_test.go	Tests and stubs for tag parsing, refresh-before-notify behavior, fallback on refresh failure, and captured event assertions.
Automations sampling changes internal/services/automations/service.go	Added automationSummary.addTorrentSamples, threaded torrentByHash through rule application (updated applySpeedLimits signature) and record torrent-name samples across many action paths.
Automations tests internal/services/automations/summary_test.go	Tests verifying non-delete action samples and addTorrentSamples deduplication/limit behavior.
Notifiarr API gating internal/services/notifications/notifiarr_api.go, internal/services/notifications/notifiarr_api_test.go	Introduce hasTorrentContext to gate population of torrent metric fields; test ensures zero-value metrics are present when context exists.
Notification formatting internal/services/notifications/service.go, internal/services/notifications/service_format_test.go	Added formatTorrentMetricLines, human-readable/raw metric modes, and included metric lines in TorrentAdded/Completed formatting; tests validate outputs.
Docs / collaboration notes AGENTS.md	Added owner collaboration notes clarifying agent review interaction rules.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Scheduler as Timer/Scheduler
    participant TorrentSync
    participant Notifier

    Caller->>Scheduler: notifyTorrentAddedWithDelay(ctx, instanceID, torrent)
    Scheduler->>Scheduler: wait torrentAddedNotificationDelay
    Scheduler->>TorrentSync: refreshTorrentForNotification(hash) (timeout torrentAddedRefreshTimeout)
    alt Refresh success
        TorrentSync-->>Scheduler: refreshedTorrent
        Scheduler->>Notifier: emit EventTorrentAdded (refreshed data)
    else Refresh failed
        TorrentSync-->>Scheduler: failure
        Scheduler->>Notifier: emit EventTorrentAdded (initial snapshot)
    end
    Notifier-->>Caller: notification dispatched

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• feat(notifications): add shoutrrr and notifiarr #1371: Refactors and centralizes torrent-added/completed notification construction and scheduling — closely related to the notification helpers here.
• fix(automations): improve applied action summaries #1478: Modifies automations service and applySpeedLimits plumbing — related to threading torrentByHash and sampling changes.

Suggested labels

notifications, tests, backend

Poem

🐇 I nudged the clock, refreshed the seed,
Trimmed tags and chased the tracker lead.
Samples gathered, metrics in sight,
A tiny hop to send it right.
📬✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly and clearly summarizes the main changes: improving torrent and automation event samples through delayed notifications, metric preservation, and expanded sampling.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch codex/notifications-automation-samples

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e514bcf773

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (3)

internal/services/notifications/notifiarr_api_test.go (1)

170-214: Consider merging torrent-field tests into a table-driven case.

This test is solid, but combining it with the existing non-zero metrics test in a table-driven pattern would reduce duplication and align with repo test style.

As per coding guidelines, "Prefer table-driven test cases and reuse integration fixtures in internal/qbittorrent/".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/services/notifications/notifiarr_api_test.go` around lines 170 -
214, Combine the zero-value test
(TestBuildNotifiarrAPIDataIncludesZeroValueTorrentMetrics) and the existing
non-zero-metrics test into a single table-driven test that iterates cases with a
name and Event input, runs svc.buildNotifiarrAPIData for each case inside t.Run,
and asserts the same set of torrent field expectations per case; locate the
Event struct instantiations and the buildNotifiarrAPIData call to create a
[]struct{ name string; event Event; } cases slice, loop over cases, and move the
repeated require.* assertions into the loop so both zero and non-zero metric
scenarios are covered without duplication.

internal/services/automations/summary_test.go (1)

138-168: Consider table-driven cases for the new sampling tests.

These two tests are very similar in shape; a small table would reduce duplication and ease future additions.

As per coding guidelines, "Prefer table-driven test cases and reuse integration fixtures in internal/qbittorrent/".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/services/automations/summary_test.go` around lines 138 - 168,
Replace the two nearly identical tests with a single table-driven test that
iterates cases (e.g., name, setup type: call recordActivity with
models.ActivityAction*/Outcome*/TorrentName or call addTorrentSamples with the
sample list and limit, and expected substrings) and for each case create
newAutomationSummary(), perform the setup (recordActivity or addTorrentSamples),
call summary.message(), and assert the expected substrings are present;
reference the existing functions newAutomationSummary(),
summary.recordActivity(), summary.addTorrentSamples(), and summary.message()
and, when applicable, reuse integration fixtures from internal/qbittorrent/ to
drive inputs rather than duplicating sample data.

internal/services/notifications/service_format_test.go (1)

13-69: Suggestion: fold the added/completed cases into a table-driven test.

Both tests share structure and assertions; a table-driven format would cut repetition and keep future additions simpler.

As per coding guidelines, "Prefer table-driven test cases and reuse integration fixtures in internal/qbittorrent/".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/services/notifications/service_format_test.go` around lines 13 - 69,
The two nearly identical tests TestFormatEventTorrentAddedIncludesMetricLines
and TestFormatEventTorrentCompletedIncludesMetricLines should be merged into a
single table-driven test: create a slice of cases (each with a name, Event input
using EventTorrentAdded or EventTorrentCompleted, expected title, and expected
substrings), instantiate svc := &Service{} once, loop cases with
t.Run(case.name, func(t *testing.T) { t.Parallel(); title, message :=
svc.formatEvent(context.Background(), case.event); require.Equal(t,
case.expectedTitle, title); for _, sub := range case.expectedSubstrings {
require.Contains(t, message, sub) } }), and remove the two original functions;
reference the Service.formatEvent method and the Event constants
EventTorrentAdded and EventTorrentCompleted when locating code to change.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@cmd/qui/torrent_notifications.go`:
- Around line 27-36: buildTorrentCompletedEvent currently returns a
notifications.Event without the metric fields formatEvent expects for
completed-torrent lines; update buildTorrentCompletedEvent to populate the same
metric fields you set for added events on the notifications.Event it returns
(use the incoming torrent and syncManager to fill metrics such as the torrent
size, bytes completed/downloaded, upload bytes/ratio, completion timestamp, and
any ETA/transfer-rate fields your formatEvent emits). Locate
buildTorrentCompletedEvent and the notifications.Event struct construction and
add assignments for those metric fields using torrent.* properties and any
helper functions you already use for added events so completed events no longer
show zeroed stats.

---

Nitpick comments:
In `@internal/services/automations/summary_test.go`:
- Around line 138-168: Replace the two nearly identical tests with a single
table-driven test that iterates cases (e.g., name, setup type: call
recordActivity with models.ActivityAction*/Outcome*/TorrentName or call
addTorrentSamples with the sample list and limit, and expected substrings) and
for each case create newAutomationSummary(), perform the setup (recordActivity
or addTorrentSamples), call summary.message(), and assert the expected
substrings are present; reference the existing functions newAutomationSummary(),
summary.recordActivity(), summary.addTorrentSamples(), and summary.message()
and, when applicable, reuse integration fixtures from internal/qbittorrent/ to
drive inputs rather than duplicating sample data.

In `@internal/services/notifications/notifiarr_api_test.go`:
- Around line 170-214: Combine the zero-value test
(TestBuildNotifiarrAPIDataIncludesZeroValueTorrentMetrics) and the existing
non-zero-metrics test into a single table-driven test that iterates cases with a
name and Event input, runs svc.buildNotifiarrAPIData for each case inside t.Run,
and asserts the same set of torrent field expectations per case; locate the
Event struct instantiations and the buildNotifiarrAPIData call to create a
[]struct{ name string; event Event; } cases slice, loop over cases, and move the
repeated require.* assertions into the loop so both zero and non-zero metric
scenarios are covered without duplication.

In `@internal/services/notifications/service_format_test.go`:
- Around line 13-69: The two nearly identical tests
TestFormatEventTorrentAddedIncludesMetricLines and
TestFormatEventTorrentCompletedIncludesMetricLines should be merged into a
single table-driven test: create a slice of cases (each with a name, Event input
using EventTorrentAdded or EventTorrentCompleted, expected title, and expected
substrings), instantiate svc := &Service{} once, loop cases with
t.Run(case.name, func(t *testing.T) { t.Parallel(); title, message :=
svc.formatEvent(context.Background(), case.event); require.Equal(t,
case.expectedTitle, title); for _, sub := range case.expectedSubstrings {
require.Contains(t, message, sub) } }), and remove the two original functions;
reference the Service.formatEvent method and the Event constants
EventTorrentAdded and EventTorrentCompleted when locating code to change.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cdeb8e5ce0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[coderabbitai]

🧹 Nitpick comments (1)

cmd/qui/torrent_notifications.go (1)

99-102: Minor: Redundant context.WithoutCancel call.

The caller (notifyTorrentAddedWithDelayAfter) already wraps the context with context.WithoutCancel at line 77. Applying it again here is harmless but unnecessary.

Consider simplifying to:

-	refreshCtx := context.Background()
-	if ctx != nil {
-		refreshCtx = context.WithoutCancel(ctx)
-	}
-	refreshCtx, cancel := context.WithTimeout(refreshCtx, torrentAddedRefreshTimeout)
+	refreshCtx, cancel := context.WithTimeout(ctx, torrentAddedRefreshTimeout)

This would require ensuring callers always pass a non-nil context, which is currently guaranteed by the goroutine in notifyTorrentAddedWithDelayAfter.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/qui/torrent_notifications.go` around lines 99 - 102, The refreshCtx
assignment in notifyTorrentAddedWithDelayAfter redundantly calls
context.WithoutCancel again; remove the extra context.WithoutCancel usage and
simply set refreshCtx = ctx (or assume ctx is non-nil) so you don't wrap the
context twice—update the code in notifyTorrentAddedWithDelayAfter to rely on the
caller's earlier context.WithoutCancel (ensure callers of
notifyTorrentAddedWithDelayAfter, e.g., the goroutine that spawns it, always
pass a non-nil context).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@cmd/qui/torrent_notifications.go`:
- Around line 27-37: buildTorrentCompletedEvent currently omits the metric
fields that buildTorrentAddedEvent populates, causing formatEvent to render
zeroed stats for completed torrents; update buildTorrentCompletedEvent to set
the same metrics as buildTorrentAddedEvent (copy the metric-related fields from
the torrent into the returned notifications.Event) so completed events include
Size/SizeLeft/Percent/Ratio/seed/peer counts and transfer speeds/time-remaining
(i.e., mirror the metric field assignments present in buildTorrentAddedEvent);
ensure you still call trackerDomainForTorrent and parseTorrentTags as in the
current function.

---

Nitpick comments:
In `@cmd/qui/torrent_notifications.go`:
- Around line 99-102: The refreshCtx assignment in
notifyTorrentAddedWithDelayAfter redundantly calls context.WithoutCancel again;
remove the extra context.WithoutCancel usage and simply set refreshCtx = ctx (or
assume ctx is non-nil) so you don't wrap the context twice—update the code in
notifyTorrentAddedWithDelayAfter to rely on the caller's earlier
context.WithoutCancel (ensure callers of notifyTorrentAddedWithDelayAfter, e.g.,
the goroutine that spawns it, always pass a non-nil context).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d277b9fd48

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[coderabbitai]

Actionable comments posted: 1

🤖 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/service.go`:
- Around line 505-518: The loop that scales `speed` uses a compound condition
accessing `units[i]` (units and index i) which flags a possible out-of-range
access; change the loop to iterate only on the index bound (e.g., for i := 1; i
< len(units); i++) and inside the body break when `speed < 1000`, updating
`speed` and setting `unit = units[i]` only after ensuring i is within range;
keep the final B/s special-case using the original `value` and return the
formatted string as before (update the loop in the same function where `units`,
`speed`, `unit` are defined).

[coderabbitai]

Actionable comments posted: 1

🤖 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/service.go`:
- Around line 178-181: The current guard in the send loop uses
s.formatEvent(...) and skips sending when message is blank, which drops valid
title-only notifications (formatCustomEvent returns title, "" for custom
events); update the guard in the loop that calls s.formatEvent (around the
title, message := s.formatEvent(ctx, event, ...)) to only continue/skip when
both strings.TrimSpace(title) == "" && strings.TrimSpace(message) == "" so
title-only notifications (e.g. EventCrossSeed* and EventAutomations* produced by
formatCustomEvent) are sent.