feat(automations): add export to instance action#1736
Open
nitrobass24 wants to merge 41 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new "Export to Instance" automation action: backend models, API validation, rule evaluation, async export execution, activity recording, tests, and frontend UI/typing updates to configure and display export operations. Changes
Sequence Diagram(s)sequenceDiagram
participant Rule as Automation Rule
participant Proc as Rule Processor
participant Svc as Automation Service
participant Src as Source Instance
participant Tgt as Target Instance
participant DB as Activity DB
Rule->>Proc: Match torrent & evaluate conditions
Proc->>Proc: Check ExportToInstance enabled & targetID valid
alt Condition met
Proc->>Svc: Queue export (hash, source, target, metadata)
Svc->>Src: ExportTorrent (via syncManager)
Src-->>Svc: Torrent payload
Svc->>Tgt: AddTorrent (options: savePath, category, tags, paused, skip_checking)
alt Add success
Tgt-->>Svc: OK
Svc->>DB: Record activity (OutcomeSuccess, targetInstanceId, savePath)
else Add failure
Tgt-->>Svc: Error
Svc->>DB: Record activity (OutcomeFailed, reason)
end
else Condition not met
Proc->>Proc: Increment condition-not-met stat
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
nitrobass24
force-pushed
the
feat/export-to-instance-action
branch
2 times, most recently
from
a45b884 to
93b4dad
Compare
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/src/components/instances/preferences/WorkflowDialog.tsx (2)
993-1005:⚠️ Potential issue | 🟠 MajorPersisted export conditions are not re-hydrated into
actionCondition.When editing a rule that only uses
conditions.exportToInstance.condition, the shared condition is dropped because the selection chain at Line 993-Line 1005 does not includeexportToInstance. Saving the rule can silently remove its condition.💡 Suggested fix
actionCondition = conditions.speedLimits?.condition ?? conditions.shareLimits?.condition ?? conditions.pause?.condition ?? conditions.resume?.condition ?? conditions.recheck?.condition ?? conditions.reannounce?.condition ?? conditions.delete?.condition ?? conditions.tags?.[0]?.condition ?? conditions.tag?.condition ?? conditions.category?.condition ?? conditions.move?.condition ?? conditions.externalProgram?.condition + ?? conditions.exportToInstance?.condition ?? nullAlso applies to: 1078-1086
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/instances/preferences/WorkflowDialog.tsx` around lines 993 - 1005, The actionCondition selection chain is missing the exportToInstance condition so rules that only use conditions.exportToInstance.condition get dropped; update the fallback chain used to compute actionCondition (the expression assigning actionCondition in WorkflowDialog.tsx) to include conditions.exportToInstance?.condition in the appropriate position (and make the same change for the second occurrence around where actionCondition is computed again at the 1078–1086 block), so that exportToInstance conditions are considered when re-hydrating the shared condition.
2558-2607:⚠️ Potential issue | 🟠 Major
Export to instanceis missing from the initial action selector.From an empty workflow (
enabledActionsCount === 0), users cannot pick export as the first action because the select list does not includeexportToInstance. This blocks creation of export-only workflows.💡 Suggested fix
<SelectContent> <SelectItem value="speedLimits">Speed limits</SelectItem> <SelectItem value="shareLimits">Share limits</SelectItem> <SelectItem value="pause">Pause</SelectItem> <SelectItem value="resume">Resume</SelectItem> <SelectItem value="recheck">Force recheck</SelectItem> <SelectItem value="reannounce">Force reannounce</SelectItem> <SelectItem value="tag">Tag</SelectItem> <SelectItem value="category">Category</SelectItem> <SelectItem value="move">Move</SelectItem> <SelectItem value="externalProgram">Run external program</SelectItem> + <SelectItem value="exportToInstance">Export to instance</SelectItem> <SelectItem value="delete" className="text-destructive focus:text-destructive">Delete (standalone only)</SelectItem> </SelectContent>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/instances/preferences/WorkflowDialog.tsx` around lines 2558 - 2607, The initial-action Select rendered when enabledActionsCount === 0 is missing the "exportToInstance" option, preventing users from creating export-only workflows; add a SelectItem with value "exportToInstance" (labelled e.g. "Export to instance") to the SelectContent used in that branch so the onValueChange handler can set exportToInstanceEnabled via setFormState (the same action value handled by setActionEnabled/export flow) and allow creating export-only rules; locate the Select block that uses enabledActionsCount === 0, onValueChange, and setFormState to insert the new SelectItem.
🧹 Nitpick comments (4)
.github/workflows/ci.yml (2)
37-39: Consider using--frozen-lockfilefor consistent CI behavior.The
webjob (line 158) usespnpm install --frozen-lockfile, but this job uses plainpnpm install. For CI consistency and reproducibility, consider using--frozen-lockfilehere as well to ensure no lockfile modifications occur during lint.Suggested fix
- name: Install dependencies working-directory: web - run: pnpm install + run: pnpm install --frozen-lockfile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 37 - 39, The CI "Install dependencies" step currently runs "pnpm install" which can modify the lockfile; change the step so the command uses the frozen lockfile flag (replace the "run: pnpm install" invocation with "run: pnpm install --frozen-lockfile") so the job enforces the lockfile and matches the other job that uses "--frozen-lockfile"; update the step that includes the "working-directory: web" and the "run: pnpm install" command accordingly.
144-154: Inconsistent action and Node.js versions across jobs.This job uses different versions than the lint jobs:
actions/checkout@v4vs@v6in lint-web/lint-backendactions/setup-node@v4vs@v6in lint-webnode-version: "22"vs"22.18.0"in lint-webConsider aligning versions across all jobs for consistency and to avoid subtle behavioral differences.
Suggested fix
- - uses: actions/checkout@v4 + - uses: actions/checkout@v6 with: ref: ${{ inputs.branch }} - - uses: actions/setup-node@v4 + - uses: actions/setup-node@v6 with: - node-version: "22" + node-version: "22.18.0" - - uses: pnpm/action-setup@v4 + - uses: pnpm/action-setup@v5 with: version: latest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 144 - 154, Update this CI job to use the same action versions and Node.js string as the lint jobs: change actions/checkout to actions/checkout@v6, change actions/setup-node to actions/setup-node@v6, and set node-version to "22.18.0" (keep pnpm/action-setup@v4 if that already matches other jobs); ensure these changes are applied in the block where actions/checkout, actions/setup-node, and node-version are declared so all jobs are consistent.internal/services/automations/service_test.go (1)
2069-2069: Consider a helper to buildrecordDryRunActivitiestest arguments.These call sites are now very signature-order sensitive due to many positional
nils; one helper/builder would make future signature changes safer and easier to review.Also applies to: 2118-2118, 2212-2212, 2265-2265, 2328-2328, 2429-2429, 2475-2475
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/automations/service_test.go` at line 2069, The many test calls to recordDryRunActivities use long positional argument lists with multiple nils and are brittle; create a helper/builder (e.g., buildRecordDryRunArgs or makeRecordDryRunActivitiesArgs) that accepts only named parameters or options and returns the full argument tuple used by recordDryRunActivities, then replace the call sites (the instances feeding recordDryRunActivities in these tests) to use the helper so future signature changes only require updating the builder; reference the recordDryRunActivities call sites and update all affected tests mentioned to use the new helper.web/src/components/instances/preferences/WorkflowDialog.tsx (1)
145-146: Add an explicit dry-run summary branch forexported_to_instance.
DRY_RUN_ACTION_LABELSsupports this action, butformatDryRunEventSummaryfalls back to"Dry-run completed"for export events. Adding a dedicated case keeps summaries consistent with other action types.Also applies to: 175-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/instances/preferences/WorkflowDialog.tsx` around lines 145 - 146, The dry-run summary falls back to "Dry-run completed" for export events even though DRY_RUN_ACTION_LABELS includes exported_to_instance; update the formatDryRunEventSummary function to add an explicit case for the exported_to_instance action (matching how other actions are handled) and return a summary that uses DRY_RUN_ACTION_LABELS.exported_to_instance (or the same phrase used elsewhere, e.g., "Export to instance") so export dry-runs produce a consistent, specific summary; ensure the new branch mirrors the structure/format used for other action types in formatDryRunEventSummary.
🤖 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/api/handlers/automations.go`:
- Around line 343-351: The ExportToInstance.Condition branch
(payload.Conditions.ExportToInstance) is being validated for target IDs but not
included in the conditionTreesForValidation/collectConditionRegexErrors pass, so
grouped-condition IDs and regex patterns can bypass validation; update the
validation logic that builds conditionTreesForValidation and calls
collectConditionRegexErrors to include
payload.Conditions.ExportToInstance.Condition (and equivalent checks referenced
around lines 500-501) so its regex/group-ID rules are validated and any errors
are returned like other condition types. Ensure you reference
payload.Conditions.ExportToInstance when adding to the trees and pass it into
collectConditionRegexErrors so export conditions fail fast during request
validation.
In `@internal/models/automation.go`:
- Around line 1021-1028: The Validate method on ExportToInstanceAction currently
only checks TargetInstanceID; extend ExportToInstanceAction.Validate to also
verify ContentLayout before persisting by (1) ensuring ContentLayout is
non-empty when Enabled is true (or when the action will be used) and (2)
validating it against the canonical set of allowed layout values (use the
existing enum/consts or a central helper — if none exists, add a small
validation helper that lists allowed layout names) so typos are rejected; update
ExportToInstanceAction.Validate to return an error describing an invalid
ContentLayout (reference the ExportToInstanceAction type and its Validate method
so the change prevents invalid ContentLayout from being forwarded by
internal/services/automations/service.go).
In `@internal/services/automations/service.go`:
- Around line 5795-5902: The code spawns an unbounded goroutine per matched
torrent (the anonymous goroutine block that calls s.syncManager.ExportTorrent
and s.syncManager.AddTorrent), risking overload; change this to use a bounded
worker pool or semaphore (e.g., a buffered channel or a worker pool goroutine
set) to limit concurrent export/add/sync operations and apply a per-export
context with timeout when calling ExportTorrent and AddTorrent (create ctx,
cancel := context.WithTimeout(parentCtx, exportTimeout) and pass that ctx into
ExportTorrent/AddTorrent), and ensure goroutines acquire/release the semaphore
around the work (and cancel the timeout on completion) so the sync_manager
export+add+sync sequence is back-pressured and cannot run unbounded
concurrently.
In `@web/src/components/instances/preferences/WorkflowsOverview.tsx`:
- Around line 191-192: The action "exported_to_instance" is only half-wired —
add it to the activity summary formatter, outcome-label mapping, and
summary-click behavior so exported rows show the same human-readable summary,
correct success/failure outcome wording, and the same click/navigation as other
batch actions; specifically, update the switch/case that builds summaries (e.g.,
formatActivitySummary/formatSummary) to handle activity.type ===
"exported_to_instance", add the corresponding branch in the outcome label logic
(e.g., getOutcomeLabel/renderOutcomeLabel) to map success/failure to the same
texts/colors used by other export/delete actions, and extend the summary click
handler (e.g., handleSummaryClick/onSummaryClick) to treat successful
"exported_to_instance" runs the same way as other successful batch runs (trigger
the same navigation/open behavior).
---
Outside diff comments:
In `@web/src/components/instances/preferences/WorkflowDialog.tsx`:
- Around line 993-1005: The actionCondition selection chain is missing the
exportToInstance condition so rules that only use
conditions.exportToInstance.condition get dropped; update the fallback chain
used to compute actionCondition (the expression assigning actionCondition in
WorkflowDialog.tsx) to include conditions.exportToInstance?.condition in the
appropriate position (and make the same change for the second occurrence around
where actionCondition is computed again at the 1078–1086 block), so that
exportToInstance conditions are considered when re-hydrating the shared
condition.
- Around line 2558-2607: The initial-action Select rendered when
enabledActionsCount === 0 is missing the "exportToInstance" option, preventing
users from creating export-only workflows; add a SelectItem with value
"exportToInstance" (labelled e.g. "Export to instance") to the SelectContent
used in that branch so the onValueChange handler can set exportToInstanceEnabled
via setFormState (the same action value handled by setActionEnabled/export flow)
and allow creating export-only rules; locate the Select block that uses
enabledActionsCount === 0, onValueChange, and setFormState to insert the new
SelectItem.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 37-39: The CI "Install dependencies" step currently runs "pnpm
install" which can modify the lockfile; change the step so the command uses the
frozen lockfile flag (replace the "run: pnpm install" invocation with "run: pnpm
install --frozen-lockfile") so the job enforces the lockfile and matches the
other job that uses "--frozen-lockfile"; update the step that includes the
"working-directory: web" and the "run: pnpm install" command accordingly.
- Around line 144-154: Update this CI job to use the same action versions and
Node.js string as the lint jobs: change actions/checkout to actions/checkout@v6,
change actions/setup-node to actions/setup-node@v6, and set node-version to
"22.18.0" (keep pnpm/action-setup@v4 if that already matches other jobs); ensure
these changes are applied in the block where actions/checkout,
actions/setup-node, and node-version are declared so all jobs are consistent.
In `@internal/services/automations/service_test.go`:
- Line 2069: The many test calls to recordDryRunActivities use long positional
argument lists with multiple nils and are brittle; create a helper/builder
(e.g., buildRecordDryRunArgs or makeRecordDryRunActivitiesArgs) that accepts
only named parameters or options and returns the full argument tuple used by
recordDryRunActivities, then replace the call sites (the instances feeding
recordDryRunActivities in these tests) to use the helper so future signature
changes only require updating the builder; reference the recordDryRunActivities
call sites and update all affected tests mentioned to use the new helper.
In `@web/src/components/instances/preferences/WorkflowDialog.tsx`:
- Around line 145-146: The dry-run summary falls back to "Dry-run completed" for
export events even though DRY_RUN_ACTION_LABELS includes exported_to_instance;
update the formatDryRunEventSummary function to add an explicit case for the
exported_to_instance action (matching how other actions are handled) and return
a summary that uses DRY_RUN_ACTION_LABELS.exported_to_instance (or the same
phrase used elsewhere, e.g., "Export to instance") so export dry-runs produce a
consistent, specific summary; ensure the new branch mirrors the structure/format
used for other action types in formatDryRunEventSummary.
🪄 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: ef23e4a1-904d-40b4-9c08-4eec4c4f25fc
📒 Files selected for processing (13)
.github/workflows/ci.yml.github/workflows/lint.yml.github/workflows/test.ymlinternal/api/handlers/automations.gointernal/models/automation.gointernal/models/automation_activity.gointernal/services/automations/processor.gointernal/services/automations/service.gointernal/services/automations/service_test.goweb/src/components/instances/preferences/AutomationActivityRunDialog.tsxweb/src/components/instances/preferences/WorkflowDialog.tsxweb/src/components/instances/preferences/WorkflowsOverview.tsxweb/src/types/index.ts
💤 Files with no reviewable changes (2)
- .github/workflows/lint.yml
- .github/workflows/test.yml
nitrobass24
force-pushed
the
feat/export-to-instance-action
branch
from
93b4dad to
67a0578
Compare
Add a new automation action that exports a torrent's .torrent file from one qBittorrent instance and adds it to another instance. Designed for migrating torrents from a seedbox to a local instance for long-term seeding where the data already exists on the target (via rclone, etc.). The action supports save path templates, category/tags on the target, skip_checking (default true), and paused mode. Execution is async (fire-and-forget) following the ExternalProgramAction pattern. Cross-seed group members are not exported — users chain this with existing Category/Tag actions using GroupID for group-level updates.
nitrobass24
force-pushed
the
feat/export-to-instance-action
branch
from
67a0578 to
5c247ae
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/models/automation.go (1)
1027-1035:⚠️ Potential issue | 🟠 MajorValidate
ContentLayoutin model validation to prevent runtime-only failures.On Line 1028,
Validate()accepts anyContentLayoutstring. Invalid values can be persisted and only fail later during execution.Suggested fix
func (a *ExportToInstanceAction) Validate() error { if a == nil { return nil } if a.Enabled && a.TargetInstanceID <= 0 { return errors.New("enabled export to instance action requires valid targetInstanceId") } + if a.Enabled { + switch strings.TrimSpace(a.ContentLayout) { + case "", "Original", "Subfolder", "NoSubfolder": + default: + return fmt.Errorf("enabled export to instance action has invalid contentLayout %q", a.ContentLayout) + } + } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/models/automation.go` around lines 1027 - 1035, ExportToInstanceAction.Validate currently doesn't verify the ContentLayout field, allowing invalid layouts to be persisted; update ExportToInstanceAction.Validate to check a.ContentLayout (when non-empty) against the canonical set of allowed content layout values (use the existing ContentLayout constants or an existing helper like ValidateContentLayout if available) and return an error when the value is not one of the accepted enums; keep the existing nil/Enabled/TargetInstanceID checks and add this ContentLayout validation branch inside the same Validate method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/models/automation.go`:
- Around line 1027-1035: ExportToInstanceAction.Validate currently doesn't
verify the ContentLayout field, allowing invalid layouts to be persisted; update
ExportToInstanceAction.Validate to check a.ContentLayout (when non-empty)
against the canonical set of allowed content layout values (use the existing
ContentLayout constants or an existing helper like ValidateContentLayout if
available) and return an error when the value is not one of the accepted enums;
keep the existing nil/Enabled/TargetInstanceID checks and add this ContentLayout
validation branch inside the same Validate method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0d8f5097-b4d5-47ce-9445-d1cb73b81e25
📒 Files selected for processing (10)
internal/api/handlers/automations.gointernal/models/automation.gointernal/models/automation_activity.gointernal/services/automations/processor.gointernal/services/automations/service.gointernal/services/automations/service_test.goweb/src/components/instances/preferences/AutomationActivityRunDialog.tsxweb/src/components/instances/preferences/WorkflowDialog.tsxweb/src/components/instances/preferences/WorkflowsOverview.tsxweb/src/types/index.ts
✅ Files skipped from review due to trivial changes (3)
- web/src/components/instances/preferences/WorkflowsOverview.tsx
- internal/models/automation_activity.go
- internal/services/automations/service_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/api/handlers/automations.go
- web/src/components/instances/preferences/AutomationActivityRunDialog.tsx
- internal/services/automations/processor.go
- web/src/components/instances/preferences/WorkflowDialog.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/services/automations/service.go (2)
5868-5869:context.Background()usage is intentional for fire-and-forget but warrants a comment.The static analysis tool flags this as G118, but based on the codebase patterns (e.g., backup service background downloads), fire-and-forget with
context.Background()is acceptable here since the parent context may be cancelled before async operations complete. Consider adding a brief comment explaining this design choice to suppress future warnings and clarify intent.for _, exec := range executions { go func() { - ctx := context.Background() + // Use Background context: parent may be cancelled before async export completes. + // Orphaned exports completing after run deletion are acceptable (fire-and-forget). + ctx := context.Background() //nolint:gosec // G118: intentional fire-and-forgetBased on learnings: "In qui's backup service, background torrent downloads initiated during manifest import intentionally use a fire-and-forget pattern with the shared service context. Per-run cancellation is not needed, as orphaned downloads completing after run deletion are considered harmless and acceptable."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/automations/service.go` around lines 5868 - 5869, Add a brief inline comment explaining that using context.Background() inside the fire-and-forget goroutine (the goroutine that sets ctx := context.Background()) is intentional: this async work is allowed to outlive the parent request/run and per-run cancellation is unnecessary because orphaned operations completing after run deletion are harmless; this documents the design choice and suppresses G118 warnings from static analysis.
5900-5920: Consider validatingTargetInstanceIDbefore attempting the add operation.While the API layer should validate this, a defensive check here would provide clearer error messages and fail faster if an invalid target somehow reaches execution.
+ // Validate target instance exists before attempting add + if exec.action.TargetInstanceID <= 0 { + log.Error(). + Int("sourceInstanceID", sourceInstanceID). + Int("targetInstanceID", exec.action.TargetInstanceID). + Str("hash", exec.hash). + Msg("automations: invalid target instance ID for export") + return + } + // 2. Build options for AddTorrent on target options := map[string]string{ "autoTMM": "false", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/automations/service.go` around lines 5900 - 5920, Add a defensive validation for the target instance before performing the add operation: check exec.action.TargetInstanceID (or exec.TargetInstanceID if that's the field used for the add) is present and valid (non-empty and/or non-zero ID, and parseable to the expected type) and return a clear error immediately if it is not; place this check near the options construction block (around where options := map[string]string{...} is set) so the add call never runs with an invalid TargetInstanceID and the error message references TargetInstanceID and the add operation context.
🤖 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/models/automation_activity.go`:
- Around line 32-34: The constant block in automation_activity.go has misaligned
spacing around the equals and comments; reformat the constants
ActivityActionAutoManaged, ActivityActionExportedToInstance, and
ActivityActionDryRunNoMatch so spacing is consistent (run gofmt or align the
equals and comments uniformly) to satisfy gofmt checks and restore consistent
alignment across the constant declarations.
In `@internal/models/automation.go`:
- Around line 874-877: Struct field alignment in the struct containing
ExternalProgram, AutoManagement, and ExportToInstance is misformatted and
failing gofmt; run gofmt -w on the file (or reformat the struct so tags align)
to fix formatting, ensuring the fields ExternalProgram *ExternalProgramAction,
AutoManagement *AutoManagementAction, and ExportToInstance
*ExportToInstanceAction use consistent spacing/indentation and correct JSON
tags.
---
Nitpick comments:
In `@internal/services/automations/service.go`:
- Around line 5868-5869: Add a brief inline comment explaining that using
context.Background() inside the fire-and-forget goroutine (the goroutine that
sets ctx := context.Background()) is intentional: this async work is allowed to
outlive the parent request/run and per-run cancellation is unnecessary because
orphaned operations completing after run deletion are harmless; this documents
the design choice and suppresses G118 warnings from static analysis.
- Around line 5900-5920: Add a defensive validation for the target instance
before performing the add operation: check exec.action.TargetInstanceID (or
exec.TargetInstanceID if that's the field used for the add) is present and valid
(non-empty and/or non-zero ID, and parseable to the expected type) and return a
clear error immediately if it is not; place this check near the options
construction block (around where options := map[string]string{...} is set) so
the add call never runs with an invalid TargetInstanceID and the error message
references TargetInstanceID and the add operation context.
🪄 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: e1bb5128-0083-4eed-a481-71150fb566fd
📒 Files selected for processing (10)
internal/api/handlers/automations.gointernal/models/automation.gointernal/models/automation_activity.gointernal/services/automations/processor.gointernal/services/automations/service.gointernal/services/automations/service_test.goweb/src/components/instances/preferences/AutomationActivityRunDialog.tsxweb/src/components/instances/preferences/WorkflowDialog.tsxweb/src/components/instances/preferences/WorkflowsOverview.tsxweb/src/types/index.ts
✅ Files skipped from review due to trivial changes (2)
- web/src/components/instances/preferences/AutomationActivityRunDialog.tsx
- web/src/components/instances/preferences/WorkflowsOverview.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/services/automations/service_test.go
- internal/api/handlers/automations.go
- web/src/components/instances/preferences/WorkflowDialog.tsx
- Fix gofmt alignment in automation.go and automation_activity.go - Remove unnecessary loop variable capture (Go 1.22+ semantics) - Suppress gosec G118 for intentional background context in goroutine - Add ExportToInstance to conditionTreesForValidation and collectConditionRegexErrors for proper regex/groupID validation - Add exportToInstance to actionCondition hydration chain - Add Export to instance option in initial action selector - Add exported_to_instance case to formatDryRunEventSummary
- Add semaphore (max 5 concurrent) and 2min timeout to export goroutines to prevent unbounded HTTP connections to qBit API - Add exported_to_instance to runSummaryActions set for summary click behavior - Add formatExportedToInstanceSummary for human-readable activity summaries - Wire exported_to_instance into summary ternary chain in activity list - Fix outcome label to show "Exported" instead of "Removed" for exports
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
internal/services/automations/service.go (1)
5867-5875:⚠️ Potential issue | 🟠 MajorThe 5-slot limiter still doesn't back-pressure dispatch.
The token is acquired inside the goroutine, so a large match set still spawns one blocked goroutine per export, and every automation run gets a fresh 5-slot queue. If the goal is a real cap, take the slot before
goor move the limiter ontoServiceso overlapping runs share it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/automations/service.go` around lines 5867 - 5875, The current limiter (maxConcurrentExports / sem) is acquired inside the goroutine so each execution spawns a blocked goroutine; to enforce a real global cap either acquire the slot before launching the goroutine (move the sem <- struct{}{} to just before the go func call and defer release inside the goroutine) or promote the limiter to a shared field on Service (e.g., add a sem channel on Service and use it across runs) so overlapping automation runs share the same limiter; update references to sem and maxConcurrentExports accordingly (or add Service.sem) and ensure defer func() { <-sem } remains inside the goroutine to release the slot.
🤖 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/automations/service.go`:
- Around line 2613-2629: The export-to-instance action is never scanned for
field-specific runtime usage so its conditions don't trigger rulesUseCondition
and EvalContext stays incomplete; update the code handling
state.exportToInstance (the block building pendingExportToInstance and appending
to exportExecutions) to run the same condition/field scans used elsewhere (e.g.,
call actionConditionsUseField or include the export action in the
rulesUseCondition path) so any fields referenced by the export action
(FREE_SPACE, cross-instance match, missing-files, hardlink scope, etc.) will
populate the EvalContext before the export is queued; ensure you reference
state.exportToInstance, resolveMovePath, exportExecutions,
pendingExportToInstance, actionConditionsUseField, and rulesUseCondition when
adding this scan so behavior matches other action types.
- Around line 3662-3663: The code dispatches s.executeExportToInstance(ctx,
instanceID, exportExecutions) while pendingByHash may already include hashes
slated for deletion, so an export and delete can race nondeterministically; pick
a deterministic precedence and enforce it before calling
s.executeExportToInstance: either remove all hashes that appear in the delete
set from exportExecutions (filter exportExecutions against pendingByHash/delete
set) or block deletes until exports finish (sequence export completion before
scheduling deletes), and update the logic around pendingByHash, exportExecutions
and the call site of s.executeExportToInstance (and any ExportTorrent caller) to
reflect that chosen behavior. Ensure the chosen approach consistently references
the same identifiers (pendingByHash, exportExecutions,
s.executeExportToInstance, ExportTorrent) so the same torrent cannot be
concurrently queued for both export and delete.
- Around line 5877-5878: The code is reusing the long exportTimeout context for
activity persistence which causes deadline/cancel errors to be recorded against
an already-done context and silently drops insert errors; update each activity
Create call (the calls that currently pass ctx from ExportTorrent/AddTorrent and
related blocks) to use a fresh short-lived background context (e.g.,
context.WithTimeout(context.Background(), a small persistenceTimeout) or
context.Background()) for the DB/Store Create operations, capture and handle the
returned error instead of discarding it, and log failures with context (use
processLogger or the existing logger) so Create errors are not silent; apply the
same change to the other affected blocks referenced (around the regions
corresponding to 5892-5904, 5942-5954, 5967-5982).
---
Duplicate comments:
In `@internal/services/automations/service.go`:
- Around line 5867-5875: The current limiter (maxConcurrentExports / sem) is
acquired inside the goroutine so each execution spawns a blocked goroutine; to
enforce a real global cap either acquire the slot before launching the goroutine
(move the sem <- struct{}{} to just before the go func call and defer release
inside the goroutine) or promote the limiter to a shared field on Service (e.g.,
add a sem channel on Service and use it across runs) so overlapping automation
runs share the same limiter; update references to sem and maxConcurrentExports
accordingly (or add Service.sem) and ensure defer func() { <-sem } remains
inside the goroutine to release the slot.
🪄 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: 01689608-ab47-4bbf-9dd7-4e4d38a4a55f
📒 Files selected for processing (2)
internal/services/automations/service.goweb/src/components/instances/preferences/WorkflowsOverview.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/components/instances/preferences/WorkflowsOverview.tsx
…sistence - Add ExportToInstance.Condition to actionConditionsUseField so fields like FREE_SPACE and cross-instance conditions populate EvalContext - Use context.Background() for activity store writes instead of the export timeout context to prevent deadline errors on slow exports - Log activity persistence failures instead of silently discarding them
nitrobass24
commented
Apr 7, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/services/automations/service.go (1)
3662-3663:⚠️ Potential issue | 🟠 MajorFilter hashes already queued for deletion out of
exportExecutions.Direct delete triggers already win because the
state.shouldDeletebranchcontinues before export collection, but hashes added topendingByHashby group/cross-seed delete expansion are never removed here. That still lets the same sibling hash be queued for fire-and-forget export and then deleted in the same run.✂️ Proposed fix
// Execute export to instance (async, fire-and-forget) -s.executeExportToInstance(ctx, instanceID, exportExecutions) +filteredExports := exportExecutions[:0] +for _, exec := range exportExecutions { + if _, deleting := pendingByHash[exec.hash]; deleting { + continue + } + filteredExports = append(filteredExports, exec) +} +s.executeExportToInstance(ctx, instanceID, filteredExports)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/automations/service.go` around lines 3662 - 3663, exportExecutions may include hashes that are already scheduled for deletion via pendingByHash because group/cross-seed expansion added them; since state.shouldDelete uses continue before export collection, you must filter out any hashes present in pendingByHash from exportExecutions before calling s.executeExportToInstance. Update the code that prepares exportExecutions (the collection point just before s.executeExportToInstance(ctx, instanceID, exportExecutions)) to remove entries whose hash exists in pendingByHash (or equivalent map), ensuring that state.shouldDelete, pendingByHash, exportExecutions, and the s.executeExportToInstance call are used consistently to locate and perform the filter.
🤖 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/automations/service.go`:
- Around line 5474-5486: The dry-run groups exportExecutions only by
exec.action.TargetInstanceID which collapses different
pendingExportToInstance.resolvedSavePath values and loses per-destination
preview; change the grouping to include resolvedSavePath (e.g., group by
targetInstanceID + resolvedSavePath or use a composite key) so you dedupe hashes
per (target, resolvedSavePath) and call createActivity with the resolvedSavePath
included in the activity data; then update the builder call
(buildRunItemsFromHashes or a new helper that accepts resolvedSavePath) to
create ActivityRunTorrent items that preserve the templated save path for each
export preview.
- Around line 5895-5908: The failure branches that log activities via
s.activityStore.Create for models.AutomationActivity currently only set Reason
and omit Details (so exports don’t record targetInstanceID/savePath); update
those failure branches (the ones where exec.ruleID/exec.hash/exec.torrent.Name
are used and Create is called) to populate the Details field with the same
payload used in the success branch (including targetInstanceID and savePath) in
addition to setting Outcome and Reason; apply the same change to the other two
similar Create calls referenced (the blocks around the other failure branches)
so failed export activities persist the destination metadata consistently.
---
Duplicate comments:
In `@internal/services/automations/service.go`:
- Around line 3662-3663: exportExecutions may include hashes that are already
scheduled for deletion via pendingByHash because group/cross-seed expansion
added them; since state.shouldDelete uses continue before export collection, you
must filter out any hashes present in pendingByHash from exportExecutions before
calling s.executeExportToInstance. Update the code that prepares
exportExecutions (the collection point just before
s.executeExportToInstance(ctx, instanceID, exportExecutions)) to remove entries
whose hash exists in pendingByHash (or equivalent map), ensuring that
state.shouldDelete, pendingByHash, exportExecutions, and the
s.executeExportToInstance call are used consistently to locate and perform the
filter.
🪄 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: 273ca455-a8ff-4ba9-ab7b-fdb7365e3817
📒 Files selected for processing (2)
internal/services/automations/service.goweb/src/components/instances/preferences/WorkflowsOverview.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/components/instances/preferences/WorkflowsOverview.tsx
Populate Details with targetInstanceId and savePath on failure activity records, matching the success path, so failed exports show where they were trying to send the torrent.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/automations/service.go`:
- Around line 2613-2629: The export append logic (where exportExecutions is
appended with pendingExportToInstance for state.exportToInstance using
resolvedSavePath, hash, torrent, action, ruleID, ruleName) will requeue the same
export repeatedly because success only logs activity; change this to first check
for a persisted completion marker or the presence of the target (e.g., a record
keyed by (hash,targetInstanceID) or checkTargetPresence(hash,
state.exportToInstance.TargetInstanceID)) and only append when not already
completed, and on successful export persist that completion marker (or
remove/mark it) so future runs skip already-exported (hash,target) pairs; apply
the same change to the other export-to-instance append site (the similar code
block later in the file).
- Around line 5919-5924: The current options map unconditionally sets
"autoTMM":"false" which forces qBittorrent to use the instance default save path
when "savepath" is not provided; change the logic in the block that builds
options (the map assigned to variable options near the handling of
exec.resolvedSavePath) so that "autoTMM":"false" is only added when
exec.resolvedSavePath is non-empty (i.e., an explicit savepath will be
provided); otherwise either omit the autoTMM key or set it to "true" to allow
category/instance automatic management.
🪄 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: 8b0f8377-2ece-47c8-b940-64d32de0b1ae
📒 Files selected for processing (2)
internal/services/automations/service.goweb/src/components/instances/preferences/WorkflowsOverview.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/components/instances/preferences/WorkflowsOverview.tsx
- Skip export when save path template resolution fails instead of using raw template string - Guard against empty torrent data from ExportTorrent - Add TrackerDomain to all export activity records - Add ActivityActionExportedToInstance to automationActionLabels for notifications - Add ExportToInstanceConditionNotMet to debug log - Acquire semaphore before spawning goroutine to limit goroutine creation - Add contentLayout field to UI (preserves round-trip, adds Original/Subfolder/NoSubfolder select) - Fix instances loading state (show "Loading..." instead of "No instances available") - Fix formatExportedToInstanceSummary count fallback to match other formatters
…ling - Check target instance for existing torrent via HasTorrentByAnyHash before queueing export, preventing repeated exports each cycle - Only set autoTMM=false when an explicit save path is provided; otherwise let qBittorrent use category/instance automatic management
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/automations/service.go`:
- Around line 5896-5900: The semaphore send currently happens before spawning
each worker in the loop, causing the main automation goroutine to block; instead
spawn the goroutine immediately for each iteration and perform the semaphore
acquisition inside the goroutine so the loop stays non-blocking. Concretely, in
the loop over executions (the block that uses sem and launches the goroutine for
worker execution) move the line that does sem <- struct{}{} into the start of
the anonymous goroutine function (before the work and with a deferred release
<-sem) so applyRulesForInstance() and other callers remain fire-and-forget and
are not blocked by the export queue.
In `@web/src/components/instances/preferences/WorkflowDialog.tsx`:
- Around line 1096-1105: The hydration code currently force-casts
conditions.exportToInstance.contentLayout into the form state
(exprExportContentLayout), which can put an unknown string into the controlled
select; instead check the value against the allowed union (the
FormState["exprExportContentLayout"]/ExportToInstanceAction.ContentLayout
options) and if it is not one of the permitted values assign "" (empty string)
so unknown values are sanitized during hydration; update the branch that sets
exprExportContentLayout to perform this validated mapping rather than using a
direct cast.
- Around line 1447-1460: buildPayload() currently drops
conditions.exportToInstance when input.exportToInstanceEnabled is true but
input.exprExportTargetInstanceId is empty; change it to fail-fast validation
instead of silently omitting the action: inside buildPayload() detect if
input.exportToInstanceEnabled && !input.exprExportTargetInstanceId and then
surface an explicit validation error (throw or return a validation result) so
the UI/save/preview path sees the missing target; reference the same shared
action validation routine if one exists (use/compose with the existing
validation function) rather than silently skipping creation of
conditions.exportToInstance.
🪄 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: 876f0182-db00-44bb-8c94-316bb7b2cb6b
📒 Files selected for processing (3)
internal/services/automations/service.goweb/src/components/instances/preferences/WorkflowDialog.tsxweb/src/components/instances/preferences/WorkflowsOverview.tsx
- Move semaphore acquire back inside goroutine so the export loop stays non-blocking and applyRulesForInstance remains fire-and-forget - Validate contentLayout against allowed values during hydration instead of unsafe cast - Throw explicit error in buildPayload when export is enabled but target instance is missing instead of silently omitting
Include both AutoManagement (from develop) and ExportToInstance in the delete-standalone mutual exclusion list.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
web/src/components/instances/preferences/WorkflowDialog.tsx (1)
1778-1781:⚠️ Potential issue | 🟠 MajorReuse export-action validation before the enable-preview flow.
These checks cover dry-run and submit, but
applyEnabledChange()still sends delete/category + export forms straight into preview. If the user flips Enabled before picking a target,buildPayload()now throws, the preview closes with an error, and the localenabledflag staystruebecause rollback only happens in the normal dialog-close path.Also applies to: 2039-2044
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/instances/preferences/WorkflowDialog.tsx` around lines 1778 - 1781, The applyEnabledChange() path can call buildPayload()/openPreview without validating export settings, causing buildPayload to throw and leaving local enabled=true; before entering the preview/dry-run flow in applyEnabledChange(), run the same export-action validation used for dry-run/submit (check dryRunInput.exportToInstanceEnabled and dryRunInput.exprExportTargetInstanceId and show toast.error("Select a target instance") and return), and additionally wrap buildPayload()/openPreview in a try/catch so that if buildPayload throws you revert the local enabled state (rollback the change) and surface an error toast; refer to applyEnabledChange, buildPayload, and the dryRunInput.exportToInstanceEnabled/exprExportTargetInstanceId checks to locate where to add the validation and the rollback logic.
🤖 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/automations/service.go`:
- Around line 2616-2649: The current branch silently skips export when
s.syncManager.HasTorrentByAnyHash returns an error or when resolveMovePath
fails; instead create and persist a failed export activity (or enqueue a
preflight failure) so these errors are recorded and handled by
executeExportToInstance. Concretely: when HasTorrentByAnyHash returns err, and
when resolveMovePath returns ok==false, construct a pendingExportToInstance-like
failure record (including hash, torrent, action: state.exportToInstance, ruleID:
state.exportToInstanceRuleID, ruleName: state.exportToInstanceRuleName, and an
error/reason string) and append it to exportExecutions (or write it to the same
failure/activity store used by executeExportToInstance) rather than simply
logging and continue; this ensures executeExportToInstance or the activity
persistence layer sees the preflight failure and dry-runs report failed exports
instead of falling through to dry_run_no_match.
In `@web/src/components/instances/preferences/WorkflowDialog.tsx`:
- Around line 1096-1108: The form currently hydrates exprExportTargetInstanceId
but never clears it if that instance is no longer in the selectable list,
causing a hidden stale ID to pass validation; update the WorkflowDialog logic
that sets exprExportTargetInstanceId (the exportToInstance handling) to either
normalize it away when the instances list finishes loading (i.e., if
exprExportTargetInstanceId is not present in the derived
availableInstances/nonSelfInstances list then set exprExportTargetInstanceId =
null) or, alternatively, ensure the dropdown rendering logic for the export
target shows a disabled “missing instance” option when the saved ID is absent;
locate the exportToInstance hydration block and the dropdown rendering code that
reads exprExportTargetInstanceId and implement one of these two fixes so the
form state and UI stay consistent.
---
Duplicate comments:
In `@web/src/components/instances/preferences/WorkflowDialog.tsx`:
- Around line 1778-1781: The applyEnabledChange() path can call
buildPayload()/openPreview without validating export settings, causing
buildPayload to throw and leaving local enabled=true; before entering the
preview/dry-run flow in applyEnabledChange(), run the same export-action
validation used for dry-run/submit (check dryRunInput.exportToInstanceEnabled
and dryRunInput.exprExportTargetInstanceId and show toast.error("Select a target
instance") and return), and additionally wrap buildPayload()/openPreview in a
try/catch so that if buildPayload throws you revert the local enabled state
(rollback the change) and surface an error toast; refer to applyEnabledChange,
buildPayload, and the
dryRunInput.exportToInstanceEnabled/exprExportTargetInstanceId checks to locate
where to add the validation and the rollback logic.
🪄 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: 0d77e9a8-1538-4a9c-8dd7-fbc29344ccf3
📒 Files selected for processing (4)
internal/api/handlers/automations.gointernal/services/automations/service.goweb/src/components/instances/preferences/WorkflowDialog.tsxweb/src/components/instances/preferences/WorkflowsOverview.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/components/instances/preferences/WorkflowsOverview.tsx
- internal/api/handlers/automations.go
…rd enable - Record failed activity when HasTorrentByAnyHash errors or save path template resolution fails instead of silently skipping - Add failureReason field to pendingExportToInstance; executeExportToInstance persists failure activity for preflight errors without spawning goroutine - Clear exprExportTargetInstanceId when instances load and the saved target is no longer in the available list - Validate export target in applyEnabledChange before entering the preview/dry-run flow to prevent buildPayload from throwing
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/api/handlers/automations.go`:
- Around line 343-351: The export-to-instance validation currently only checks
TargetInstanceID shape; update the ExportToInstance validation in automations.go
to verify the target instance actually exists by calling the instances
repository (e.g., use the existing InstanceStore/GetInstanceByID or equivalent)
with payload.Conditions.ExportToInstance.TargetInstanceID; if the lookup returns
not found, return http.StatusBadRequest with an explanatory message, and if the
lookup returns a datastore/error, return an appropriate server error (500)
rather than persisting a non-existent target; keep the existing checks for <=0
and self-export and perform the existence check after them.
In `@internal/services/automations/service.go`:
- Around line 5502-5514: The current loop over exportExecutions
(pendingExportToInstance) groups by target and drops per-execution
failureReason, causing dry-run to show successful exported_to_instance even when
preflight failed; update the code that iterates exportExecutions to separate
successful runs from failed ones (check exec.failureReason or similar on each
pendingExportToInstance) and: 1) for successful executions keep the existing
grouping -> dedupeHashes -> createActivity(ActivityActionExportedToInstance,
{...}, buildRunItemsFromHashes(...)); 2) for failed executions either create a
second activity (e.g., ActivityActionExportPreflightFailed) or include the
failureReason details in the Details map for that target (e.g., add a
"failures": [...] array with target-specific failureReason strings) so dry-run
output reflects preflight failures; reference exportExecutions, createActivity,
ActivityActionExportedToInstance, buildRunItemsFromHashes, dedupeHashes,
torrentByHash, and s.syncManager when making the change.
- Around line 2615-2657: Add an in-memory "in-flight" set keyed by
targetInstanceID+hash to avoid queuing duplicate exports before the first worker
calls AddTorrent: add fields on the service (e.g., inFlightExports
map[string]struct{} plus a mutex) and in the export block (around where
pendingExportToInstance is created and before calling
s.syncManager.HasTorrentByAnyHash) compute a key like fmt.Sprintf("%d:%s",
state.exportToInstance.TargetInstanceID, hash), check the map and skip/log if
already in-flight, otherwise insert the key before appending to
exportExecutions; ensure the key is removed when the actual export/AddTorrent
operation completes or fails so the slot is freed.
🪄 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: 0df6c630-ff50-4153-9d45-2b7b71c32321
📒 Files selected for processing (4)
internal/api/handlers/automations.gointernal/services/automations/service.goweb/src/components/instances/preferences/WorkflowDialog.tsxweb/src/components/instances/preferences/WorkflowsOverview.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/components/instances/preferences/WorkflowsOverview.tsx
- web/src/components/instances/preferences/WorkflowDialog.tsx
…up in-flight - Validate target instance exists in DB during API save (not just ID > 0) - Separate successful vs failed exports in dry-run activity recording so preflight failures show distinct from successful exports - Add inFlightExports map to prevent duplicate export queueing across overlapping automation cycles; key removed on goroutine completion
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
web/src/components/instances/preferences/WorkflowDialog.tsx (2)
3392-3410: Consider memoizing the filtered instances list.The
instances.filter(i => i.id !== instanceId)is computed twice: once for the length check (line 3392) and again for mapping (line 3404). For a small list this is negligible, but you could extract it to auseMemofor cleaner code:♻️ Optional: memoize filtered instances
const nonSelfInstances = useMemo( () => instances?.filter(i => i.id !== instanceId) ?? [], [instances, instanceId] )Then use
nonSelfInstances.length > 0andnonSelfInstances.map(...)in the JSX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/instances/preferences/WorkflowDialog.tsx` around lines 3392 - 3410, The JSX calls instances.filter(i => i.id !== instanceId) twice which duplicates work and hurts readability; extract the filtered list into a memoized variable using useMemo (e.g., const nonSelfInstances = useMemo(() => instances?.filter(i => i.id !== instanceId) ?? [], [instances, instanceId])) and then replace both occurrences (the length check and the map that renders SelectItem) with nonSelfInstances and nonSelfInstances.map(...), keeping the existing Select value/onValueChange logic intact.
3386-3415: Consider adding a loading spinner for consistency.The external programs loading state (lines 3310-3313) includes a
<Loader2>spinner, but the instances loading state here shows plain text. For UI consistency:💄 Add spinner to loading state
{instancesLoading ? ( - <div className="text-sm text-muted-foreground p-2 border rounded-md bg-muted/50"> + <div className="text-sm text-muted-foreground p-2 border rounded-md bg-muted/50 flex items-center gap-2"> + <Loader2 className="h-3.5 w-3.5 animate-spin" /> Loading instances... </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/instances/preferences/WorkflowDialog.tsx` around lines 3386 - 3415, The instances loading branch uses plain text instead of the spinner used elsewhere; replace the plain "Loading instances..." div (the instancesLoading branch in WorkflowDialog.tsx) with the same Loader2 spinner component and styling used in the external programs loading state so the UI is consistent—keep the surrounding structure (the Select, SelectTrigger, SelectContent, SelectItem and formState.exprExportTargetInstanceId logic) unchanged, only swap the plain text box for a div that includes the Loader2 icon and the same classes/appearance as the other loading block.
🤖 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/automations/service.go`:
- Around line 3708-3709: The export outcomes are recorded asynchronously by
executeExportToInstance, so notifyAutomationSummary runs too early and misses
export activity; change the flow so export results are available before calling
notifyAutomationSummary: either make executeExportToInstance return a
result/summary (or accept a callback/channel) that collects success/failure for
each export and updates the automation summary, or await completion of
background export workers (propagate context/cancellation) and merge their
outcomes into the existing summary, then call notifyAutomationSummary; update
the call site that currently does s.executeExportToInstance(ctx, instanceID,
exportExecutions) and the executeExportToInstance signature/behavior so
summary.hasActivity() reflects export outcomes before notifyAutomationSummary
runs.
- Line 535: The file contains unformatted Go code (e.g., the declaration for
inFlightExports map[string]struct{} with its comment) causing CI to fail; run
gofmt (e.g., gofmt -w) on the file to fix spacing/formatting, ensure exported
identifiers use PascalCase and locals use camelCase, and re-run formatting/lint
to verify the inFlightExports declaration and surrounding lines are gofmt-clean
before merging.
- Around line 2627-2672: The code currently inserts flightKey into
s.inFlightExports before the dry-run/preflight and using a separate RLock then
Lock, which allows dry-runs to reserve keys and causes races; change this so the
reservation is done atomically only on the live path: move the reservation
(check+insert) behind the dry-run/preflight decision (i.e., after
applyForInstance() determines not a dry-run) and perform it with s.mu.Lock()
doing the existence check and insertion in one critical section (use the same
flightKey and s.inFlightExports map); also ensure any early exits on preflight
failures or skips remove the reserved key (or never reserve for dry-run) and
keep the existing cleanup in executeExportToInstance() to delete the key on
completion so no stale entries remain.
---
Nitpick comments:
In `@web/src/components/instances/preferences/WorkflowDialog.tsx`:
- Around line 3392-3410: The JSX calls instances.filter(i => i.id !==
instanceId) twice which duplicates work and hurts readability; extract the
filtered list into a memoized variable using useMemo (e.g., const
nonSelfInstances = useMemo(() => instances?.filter(i => i.id !== instanceId) ??
[], [instances, instanceId])) and then replace both occurrences (the length
check and the map that renders SelectItem) with nonSelfInstances and
nonSelfInstances.map(...), keeping the existing Select value/onValueChange logic
intact.
- Around line 3386-3415: The instances loading branch uses plain text instead of
the spinner used elsewhere; replace the plain "Loading instances..." div (the
instancesLoading branch in WorkflowDialog.tsx) with the same Loader2 spinner
component and styling used in the external programs loading state so the UI is
consistent—keep the surrounding structure (the Select, SelectTrigger,
SelectContent, SelectItem and formState.exprExportTargetInstanceId logic)
unchanged, only swap the plain text box for a div that includes the Loader2 icon
and the same classes/appearance as the other loading block.
🪄 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: cc1c1fd4-f909-4e39-877c-b7a5ff170150
📒 Files selected for processing (4)
internal/api/handlers/automations.gointernal/services/automations/service.goweb/src/components/instances/preferences/WorkflowDialog.tsxweb/src/components/instances/preferences/WorkflowsOverview.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/components/instances/preferences/WorkflowsOverview.tsx
- internal/api/handlers/automations.go
…I polish - executeExportToInstance returns channel of activities; caller drains results into summary before notifyAutomationSummary so export outcomes appear in notifications - In-flight reservation only on live runs (not dry-run), with atomic check+insert under single Lock to prevent TOCTOU races - Memoize nonSelfInstances to avoid duplicate filter calls in JSX - Use Loader2 spinner for instance loading state matching existing UI
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/services/automations/service.go (1)
3709-3710:⚠️ Potential issue | 🟠 MajorThe synchronous drain of
exportResultsblocksapplyRulesForInstance()past the 60-secondApplyTimeoutdeadline.
executeExportToInstance()only closes its channel after all 5 workers finish (or hit the 2-minute timeout), so thefor activity := range exportResultsloop at line 3814 blocks until the full export backlog drains. Since this drain happens inside the 60-second timeout context set at the start ofapplyRulesForInstance(), busy instances can easily exceed the deadline beforenotifyAutomationSummary()runs. Sequential instance processing inapplyAll()then stalls waiting for the previous instance's export drain to complete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/automations/service.go` around lines 3709 - 3710, The current synchronous range over exportResults in applyRulesForInstance blocks the function's ApplyTimeout; change the logic so applyRulesForInstance does NOT block on draining exportResults: call executeExportToInstance to get exportResults, then launch a separate goroutine that consumes the channel (e.g., collects into a slice and sends a done signal or stores results) using its own context/timeout (or the existing 2-minute worker timeout) so the drain can run independently; then immediately proceed to notifyAutomationSummary in applyRulesForInstance and, if needed, wait for the drain goroutine via a non-blocking select or separate wait channel without using the applyRulesForInstance ApplyTimeout context. Ensure changes reference executeExportToInstance, exportResults, applyRulesForInstance, and notifyAutomationSummary so the drain no longer blocks the main 60s 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 `@internal/services/automations/service.go`:
- Around line 3709-3710: The current synchronous range over exportResults in
applyRulesForInstance blocks the function's ApplyTimeout; change the logic so
applyRulesForInstance does NOT block on draining exportResults: call
executeExportToInstance to get exportResults, then launch a separate goroutine
that consumes the channel (e.g., collects into a slice and sends a done signal
or stores results) using its own context/timeout (or the existing 2-minute
worker timeout) so the drain can run independently; then immediately proceed to
notifyAutomationSummary in applyRulesForInstance and, if needed, wait for the
drain goroutine via a non-blocking select or separate wait channel without using
the applyRulesForInstance ApplyTimeout context. Ensure changes reference
executeExportToInstance, exportResults, applyRulesForInstance, and
notifyAutomationSummary so the drain no longer blocks the main 60s context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d9a70273-231a-4ea6-b204-3f5914a30d47
📒 Files selected for processing (4)
internal/api/handlers/automations.gointernal/services/automations/service.goweb/src/components/instances/preferences/WorkflowDialog.tsxweb/src/components/instances/preferences/WorkflowsOverview.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/api/handlers/automations.go
- web/src/components/instances/preferences/WorkflowsOverview.tsx
- web/src/components/instances/preferences/WorkflowDialog.tsx
…ve path help - Fetch categories from target instance via useInstanceMetadata and render a Select dropdown instead of free-text input - Reset category when target instance changes (categories differ) - Fall back to text input when target has no categories or isn't selected - Update save path help text: clarify it's the parent directory for torrent content, leave empty to use category default, templates are optional
…state, extract layout const - Enable autoTMM when category is set but no explicit save path, so qBittorrent uses the category's configured save path on the target - Clear lastStateChecking on lookup error or not-found to prevent stale checking flag from causing false success - Extract CONTENT_LAYOUT_OPTIONS constant used by both hydration validation and UI renderer
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/automations/service.go`:
- Around line 6128-6130: The import branch only sets options["paused"] based on
exec.action.Paused which leads to inconsistent start state; update the import
logic where exec.action.Paused is checked (the block referencing
exec.action.Paused and the options map) to also set options["stopped"]="true"
when paused is true (and ensure it is cleared/omitted when paused is false) so
that both "paused" and "stopped" are controlled together consistent with
crossseed/dirscan/API/backup behavior.
In `@web/src/components/instances/preferences/WorkflowDialog.tsx`:
- Around line 3453-3455: The save-path hint in the WorkflowDialog JSX currently
lists only two placeholders; update the copy in the <p> element inside
WorkflowDialog (where it reads "Supports templates: {"{{ .Category }}"}, {"{{
.Tracker }}"}") to include all supported placeholders ({{ .Category }}, {{
.Tracker }}, {{ .Name }}, {{ .Hash }}) or alternatively change the sentence to
an "e.g." form to indicate examples; ensure the displayed text reflects the
chosen approach so users know all supported templates.
- Around line 926-929: The computed options list (targetCategories) discards the
current exprExportCategory value when targetMetadata returns categories, so the
select shows no matching option despite the value being submitted; update the
useMemo that builds targetCategories to include exprExportCategory (add it to
the dependency array) and, when targetMetadata?.categories exists, merge
Object.keys(targetMetadata.categories).sort() with exprExportCategory
(deduplicating) so the current value is present in the options; apply the same
change to the other similar computed option arrays referenced around
exprExportCategory (the other blocks noted) so saved/manually-entered values are
preserved in the option lists.
🪄 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: d1649a79-ea42-4140-be61-f160ddf1b53e
📒 Files selected for processing (4)
internal/api/handlers/automations.gointernal/services/automations/service.goweb/src/components/instances/preferences/WorkflowDialog.tsxweb/src/components/instances/preferences/WorkflowsOverview.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/components/instances/preferences/WorkflowsOverview.tsx
…ve saved category - Set options["stopped"]="true" alongside "paused" to match the pattern used by backup restore, dirscan, and cross-seed - List all supported save path templates (Name, Category, Hash, Tracker) - Merge saved exprExportCategory into targetCategories so it remains selectable even if the category doesn't exist on target yet
Contributor
There was a problem hiding this comment.
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/automations/service.go`:
- Around line 6058-6097: The activity currently uses getTrackerForTorrent()
(trackerDomain) but ExportTorrent() already returns the source torrent’s primary
tracker; update the code so ExportTorrent's third return value (e.g.,
primaryTracker) is captured and assigned to trackerDomain after calling
s.syncManager.ExportTorrent, then have buildActivity reference that
trackerDomain. Replace the earlier trackerDomain :=
getTrackerForTorrent(&exec.torrent, s.syncManager) with a fallback only (use
getTrackerForTorrent() only if ExportTorrent returns an empty/zero
primaryTracker), and change the ExportTorrent call to unpack its third return
into a variable (e.g., primaryTracker) so failed or successful export activities
use the ExportTorrent-provided primary tracker domain.
🪄 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: df025872-0cf0-4b5e-9421-07959135af7c
📒 Files selected for processing (4)
internal/api/handlers/automations.gointernal/services/automations/service.goweb/src/components/instances/preferences/WorkflowDialog.tsxweb/src/components/instances/preferences/WorkflowsOverview.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/components/instances/preferences/WorkflowsOverview.tsx
- internal/api/handlers/automations.go
Capture the third return value (trackerDomain) from ExportTorrent and use it for activity records when non-empty, falling back to getTrackerForTorrent only when ExportTorrent doesn't provide one.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/automations/service.go`:
- Around line 2640-2645: The exists branch currently logs and falls through to
the default which yields a misleading dry_run_no_match; instead propagate a
distinct "already satisfied" preview state for export-only dry-runs. In the
exists case (the switch branch labeled "case exists") set or carry a
preview/previewActivity value (e.g., "dry_run_already_satisfied" or
"dry_run_skipped_already_exists") on the state object (state.exportToInstance or
the run result structure) rather than letting execution reach the
dry_run_no_match path, and ensure the final result-selection logic that
currently maps to dry_run_no_match recognizes and preserves this new state so
the run reports "matched but already satisfied" for torrents where
state.exportToInstance.TargetInstanceID indicates the target already has the
torrent.
- Around line 3860-3887: When a drain is interrupted (mergePartial called or
drainTimer/ctx paths) ensure we add a "partial" summary entry for any pending
export-only rules so notifications aren't suppressed and reflect partial counts:
inside mergePartial (and the timeout/cancel branches) detect pending exports
from exportBatchCh/eligibleRules, increment summary rules or add a sentinel
partial rule via summary.recordActivity or summary.rules manipulation to
indicate partial=true and include currently known counts, then proceed to call
s.notifyAutomationSummary (using the existing notifyCtx fallback) so the
notification is always sent and clearly marked as partial; keep existing use of
stopDrain, exportBatchCh, summary, recordActivity, notifyAutomationSummary,
eligibleRules, drainTimer, ctx and s.cfg.ApplyTimeout to locate and integrate
the change.
🪄 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: f866c7f7-cef4-499b-b4d6-dda575e52202
📒 Files selected for processing (4)
internal/api/handlers/automations.gointernal/services/automations/service.goweb/src/components/instances/preferences/WorkflowDialog.tsxweb/src/components/instances/preferences/WorkflowsOverview.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/api/handlers/automations.go
- web/src/components/instances/preferences/WorkflowsOverview.tsx
… of no-match When a torrent already exists on the target instance, dry-run now records it as a preflight failure with "Already exists on target instance" so the report shows matched-but-satisfied instead of falling through to dry_run_no_match. Live runs continue to silently skip as before.
…fication Add savePath and contentPath to the missingFiles warning log, and include savePath in the failure reason string. Also log configured save path, category, and autoTMM state at the error call site so path mismatches are diagnosable.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/services/automations/service.go (1)
3865-3892:⚠️ Potential issue | 🟠 MajorInterrupted export waits can still suppress notifications for export-only rules.
If
ctx.Done()or the drain timeout fires before the first export activity arrives,mergePartial()folds an empty batch intosummary, so Line 3897 still short-circuitsnotifyAutomationSummary(). Export-only rules can therefore disappear completely on the exact slow/hung paths this code is trying to surface. Add a sentinel partial export entry or apartialflag wheneverlen(exportExecutions) > 0but no activities were collected before notifying.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/automations/service.go` around lines 3865 - 3892, mergePartial currently folds an empty batch into summary when ctx.Done() or drainTimer fires before any export activity arrives, allowing export-only rules to be dropped before notifyAutomationSummary is called; modify mergePartial (and the select branch that calls it) to set a sentinel/partial flag or insert a minimal "partial export" activity into summary whenever exportExecutions (or equivalent) indicates exports were expected but batch.activities is empty, then ensure notifyAutomationSummary(instanceID, summary, eligibleRules) sees that flag/activity so export-only rules are preserved and reported even on timeout or context cancellation.
🤖 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/automations/service.go`:
- Around line 5602-5620: The code currently treats any non-empty
exec.failureReason as preflight failure; change the classification in the loop
over exportExecutions so that executions whose failureReason equals the benign
"Already exists on target instance" are not placed in failedByTarget but instead
counted as success (or their own "already exists" bucket) — update the condition
that populates failedByTarget/successByTarget in the for _, exec := range
exportExecutions loop to exclude that exact message (e.g., only treat as failure
when exec.failureReason != "" && exec.failureReason != "Already exists on target
instance"), so createActivity calls (and the preflightFailed flag passed to
createActivity) only reflect true preflight failures for buildRunItemsFromHashes
and activity creation.
In `@web/src/components/instances/preferences/WorkflowDialog.tsx`:
- Around line 696-697: The category input currently treats an empty
targetCategories the same as a loading/error state because
useInstanceMetadata(...)’s targetMetadata is not distinguished; update the
rendering logic around formState.exprExportTargetInstanceId / targetMetadata and
the code that derives targetCategories so you only fall back to a plain input
when metadata has finished loading and returned zero categories. Use the
loading/error flags returned by useInstanceMetadata (or check for undefined vs
null) to render a loading indicator or disabled state while targetMetadata is
loading or show an explicit error state on failure, and only compute/choose the
“no categories” fallback when targetMetadata is successfully loaded and
targetCategories.length === 0; apply the same change to the other occurrence
around lines 3461–3488.
---
Duplicate comments:
In `@internal/services/automations/service.go`:
- Around line 3865-3892: mergePartial currently folds an empty batch into
summary when ctx.Done() or drainTimer fires before any export activity arrives,
allowing export-only rules to be dropped before notifyAutomationSummary is
called; modify mergePartial (and the select branch that calls it) to set a
sentinel/partial flag or insert a minimal "partial export" activity into summary
whenever exportExecutions (or equivalent) indicates exports were expected but
batch.activities is empty, then ensure notifyAutomationSummary(instanceID,
summary, eligibleRules) sees that flag/activity so export-only rules are
preserved and reported even on timeout or context cancellation.
🪄 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: 04563742-c619-41c5-bc7b-a6dd568c3e6d
📒 Files selected for processing (4)
internal/api/handlers/automations.gointernal/services/automations/service.goweb/src/components/instances/preferences/WorkflowDialog.tsxweb/src/components/instances/preferences/WorkflowsOverview.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/components/instances/preferences/WorkflowsOverview.tsx
- internal/api/handlers/automations.go
…ategory loading state - Dry-run now classifies "Already exists on target instance" as its own bucket (alreadyOnTarget) separate from preflight failures - Category dropdown shows loading spinner while target metadata loads, disabled state when no target is selected, and falls back to text input only when metadata has loaded with zero categories
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/services/automations/service.go (2)
6224-6285:⚠️ Potential issue | 🟡 MinorClear
lastErron any successful lookup, not justfound == true.
HasTorrentByAnyHash(...)=(_, false, nil)means the lookup succeeded but the torrent is not visible yet. BecauselastErris only reset afterfound == true, one transient poll error can survive through later cleannot foundpolls and become the final failure reason.🩹 Proposed fix
torrent, found, err := s.syncManager.HasTorrentByAnyHash(ctx, targetInstanceID, []string{hash}) if err != nil { lastErr = err lastStateChecking = false log.Debug().Err(err). Int("targetInstanceID", targetInstanceID). Str("hash", hash).Int("attempt", attempt+1). Msg("automations: verification poll failed, retrying") continue } + lastErr = nil if !found { lastStateChecking = false log.Debug(). Int("targetInstanceID", targetInstanceID). Str("hash", hash).Int("attempt", attempt+1). Msg("automations: torrent not yet visible on target, retrying") continue } - - // Successful lookup — clear transient error state - lastErr = nilAs 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/automations/service.go` around lines 6224 - 6285, The code only clears lastErr after a successful found == true case, so a prior transient err can persist across later successful HasTorrentByAnyHash calls that return (found == false, err == nil); update the !found branch in the loop (the block handling HasTorrentByAnyHash results) to explicitly set lastErr = nil (and keep lastStateChecking = false) when err == nil and !found to ensure a clean lookup clears prior errors (refer to HasTorrentByAnyHash, lastErr, lastStateChecking, skipChecking and the surrounding retry loop).
3865-3892:⚠️ Potential issue | 🟠 MajorEmit a partial export summary when the drain is interrupted.
On the timeout/cancel path this only merges finished export activities. If no worker has completed yet,
summary.hasActivity()andsummary.rulesstay empty, sonotifyAutomationSummary()returns early and export-only rules still send no notification. Please persist a partial export marker/state before notifying so interrupted runs remain visible to users.As per coding guidelines, "Prefer explicit error handling over silent failures in Go".
🧹 Nitpick comments (1)
web/src/components/instances/preferences/WorkflowsOverview.tsx (1)
821-821: Consider using a distinct badge color.The cyan color scheme matches
deleted_condition(line 807), which may reduce visual distinction between these action types at a glance. Consider using a different color like teal or another unused color to differentiate export actions from deletion-related actions.💡 Suggested change
- exported_to_instance: "bg-cyan-500/10 text-cyan-500 border-cyan-500/20", + exported_to_instance: "bg-teal-500/10 text-teal-500 border-teal-500/20",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/instances/preferences/WorkflowsOverview.tsx` at line 821, The badge color for exported_to_instance currently reuses the cyan palette used by deleted_condition; update the exported_to_instance CSS class (the exported_to_instance entry) to a distinct color (for example switch from cyan to teal by using bg-teal-500/10 text-teal-500 border-teal-500/20 or another unused color token) so export actions are visually distinguishable from deletion-related actions; keep the same utility structure when replacing the token so sizing/opacity stays consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/instances/preferences/WorkflowDialog.tsx`:
- Around line 3477-3490: The Select currently uses the literal "none" sentinel
which can collide with a real category named "none"; replace that literal with
the shared uncategorized sentinel constant used elsewhere (e.g.,
UNCATEGORIZED_SENTINEL) in the Select value, the SelectItem value, and the
onValueChange logic inside WorkflowDialog (update formState.exprExportCategory
via setFormState to set "" when the sentinel is chosen, otherwise the real
value), so the sentinel is unique and real categories named "none" round-trip
correctly.
---
Duplicate comments:
In `@internal/services/automations/service.go`:
- Around line 6224-6285: The code only clears lastErr after a successful found
== true case, so a prior transient err can persist across later successful
HasTorrentByAnyHash calls that return (found == false, err == nil); update the
!found branch in the loop (the block handling HasTorrentByAnyHash results) to
explicitly set lastErr = nil (and keep lastStateChecking = false) when err ==
nil and !found to ensure a clean lookup clears prior errors (refer to
HasTorrentByAnyHash, lastErr, lastStateChecking, skipChecking and the
surrounding retry loop).
---
Nitpick comments:
In `@web/src/components/instances/preferences/WorkflowsOverview.tsx`:
- Line 821: The badge color for exported_to_instance currently reuses the cyan
palette used by deleted_condition; update the exported_to_instance CSS class
(the exported_to_instance entry) to a distinct color (for example switch from
cyan to teal by using bg-teal-500/10 text-teal-500 border-teal-500/20 or another
unused color token) so export actions are visually distinguishable from
deletion-related actions; keep the same utility structure when replacing the
token so sizing/opacity stays consistent.
🪄 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: 21998091-85d1-4046-8a69-bdf56fcf9c50
📒 Files selected for processing (4)
internal/api/handlers/automations.gointernal/services/automations/service.goweb/src/components/instances/preferences/WorkflowDialog.tsxweb/src/components/instances/preferences/WorkflowsOverview.tsx
✅ Files skipped from review due to trivial changes (1)
- internal/api/handlers/automations.go
…unt display
Bug 1: When post-add verification fails (missingFiles, error state),
delete the torrent from the target instance via BulkAction("delete")
so it doesn't block re-export on the next automation run.
Bug 2: Per-torrent export activities were missing the "count" field
in details, causing the UI to display "0 torrents exported/failed".
Add count:1 to per-torrent activity details.
…distinct badge color - Replace "none" category sentinel with "__none__" matching the existing pattern in RSSPage to avoid collision with a real category - Clear lastErr on successful not-found lookups so transient errors don't persist across clean polls - Change exported_to_instance badge from cyan (conflicts with deleted_condition) to blue for visual distinction
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
web/src/components/instances/preferences/WorkflowDialog.tsx (1)
3478-3490:⚠️ Potential issue | 🟡 MinorUse the shared uncategorized sentinel instead of
"__none__".The literal sentinel can collide with a real category value. Reuse
CATEGORY_UNCATEGORIZED_VALUE(already imported) for consistent round-tripping.💡 Minimal fix
- <Select - value={formState.exprExportCategory || "__none__"} + <Select + value={formState.exprExportCategory === "" ? CATEGORY_UNCATEGORIZED_VALUE : formState.exprExportCategory} onValueChange={(value) => setFormState(prev => ({ ...prev, - exprExportCategory: value === "__none__" ? "" : value, + exprExportCategory: value === CATEGORY_UNCATEGORIZED_VALUE ? "" : value, }))} > @@ - <SelectItem value="__none__">No category</SelectItem> + <SelectItem value={CATEGORY_UNCATEGORIZED_VALUE}>No category</SelectItem>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/instances/preferences/WorkflowDialog.tsx` around lines 3478 - 3490, Replace the hard-coded "__none__" sentinel with the shared constant CATEGORY_UNCATEGORIZED_VALUE in the WorkflowDialog component: update the Select's value prop (currently using formState.exprExportCategory || "__none__"), the onValueChange conversion check (value === "__none__" ? "" : value), and the SelectItem that renders the "No category" option (value="__none__") to use CATEGORY_UNCATEGORIZED_VALUE so round-tripping uses the canonical uncategorized sentinel; keep the rest of the setFormState logic and SelectValue placeholder unchanged.
🧹 Nitpick comments (1)
internal/services/automations/service.go (1)
2647-2648: Extract the “already exists” reason into a shared constant.The same sentinel string is duplicated in planning and dry-run grouping. If one side changes, dry-run classification silently regresses.
♻️ Suggested refactor
+const exportAlreadyExistsReason = "Already exists on target instance" ... - exportEntry.failureReason = "Already exists on target instance" + exportEntry.failureReason = exportAlreadyExistsReason ... - const alreadyExistsReason = "Already exists on target instance" successByTarget := make(map[int][]string) ... - case exec.failureReason == alreadyExistsReason: + case exec.failureReason == exportAlreadyExistsReason:Also applies to: 5602-5611
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/automations/service.go` around lines 2647 - 2648, Extract the duplicated sentinel string "Already exists on target instance" into a single exported constant (e.g., AlreadyExistsFailureReason) in the automations package and replace all literal uses with that constant (specifically replace the assignment to exportEntry.failureReason and the duplicate usage in the planning/dry-run grouping logic referenced in the review). Update any comparisons that check the failure reason to use the constant as well so both sides share the same value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@web/src/components/instances/preferences/WorkflowDialog.tsx`:
- Around line 3478-3490: Replace the hard-coded "__none__" sentinel with the
shared constant CATEGORY_UNCATEGORIZED_VALUE in the WorkflowDialog component:
update the Select's value prop (currently using formState.exprExportCategory ||
"__none__"), the onValueChange conversion check (value === "__none__" ? "" :
value), and the SelectItem that renders the "No category" option
(value="__none__") to use CATEGORY_UNCATEGORIZED_VALUE so round-tripping uses
the canonical uncategorized sentinel; keep the rest of the setFormState logic
and SelectValue placeholder unchanged.
---
Nitpick comments:
In `@internal/services/automations/service.go`:
- Around line 2647-2648: Extract the duplicated sentinel string "Already exists
on target instance" into a single exported constant (e.g.,
AlreadyExistsFailureReason) in the automations package and replace all literal
uses with that constant (specifically replace the assignment to
exportEntry.failureReason and the duplicate usage in the planning/dry-run
grouping logic referenced in the review). Update any comparisons that check the
failure reason to use the constant as well so both sides share the same value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0d82a3de-792c-4827-977e-c00d6bffed9a
📒 Files selected for processing (4)
internal/api/handlers/automations.gointernal/services/automations/service.goweb/src/components/instances/preferences/WorkflowDialog.tsxweb/src/components/instances/preferences/WorkflowsOverview.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/components/instances/preferences/WorkflowsOverview.tsx
- internal/api/handlers/automations.go
Contributor
Author
|
Closes #1126 |
…t count 1. Before deleting a failed-verification torrent from target, re-check its state one final time. If it has become healthy (progress >= 1.0, not error/missingFiles), skip cleanup and report success. Prevents deleting a torrent that recovered after checkingResumeData timeout. 2. Verification loop now checks immediately on first iteration instead of waiting 3s. Sleep moved to end of loop body (between retries). Saves 3s per export with skip_checking=true. 3. Add count:1 to preflight failure activity details so the UI displays "1 torrent failed to export" instead of "0".
The verify loop was reading from the in-memory torrent cache without forcing a refresh, so all 10 attempts saw the same stale snapshot. Also, not-found and checking branches skipped the poll interval sleep via continue, causing retries to fire almost instantly. Force a Sync() call before each HasTorrentByAnyHash check (matching the pattern in ResumeWhenComplete) and ensure all branches fall through to the pollInterval wait.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (5)
web/src/components/instances/preferences/WorkflowDialog.tsx (2)
3423-3442:⚠️ Potential issue | 🟡 MinorRender missing target instances explicitly.
If an existing rule points at a target that has been deleted or filtered out, this select renders with no matching item while
exprExportTargetInstanceIdstays set. The dialog looks empty until submit/dry-run/enable clears it, which makes the form state misleading. Add a disabled “missing instance” option for the hydrated ID, or normalize it before rendering, so the current value stays visible.As per coding guidelines, "To reset state in React, prefer a key or render-time adjustments instead of Effects."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/instances/preferences/WorkflowDialog.tsx` around lines 3423 - 3442, The select for export target can hide a currently set exprExportTargetInstanceId when its instance was deleted/filtered, leaving the form appearing empty; update the render logic in the WorkflowDialog component around the Select (where nonSelfInstances is mapped to SelectItem and value uses formState.exprExportTargetInstanceId) to handle missing/unknown IDs at render time: check whether formState.exprExportTargetInstanceId exists in nonSelfInstances (by id), and if not either render a disabled SelectItem labeled like "Missing instance (id: ...)" with value set to the hydrated id or normalize the value to null before passing into the SelectValue; do this in the render path (not via useEffect) and use setFormState only when user changes selection via onValueChange to clear/reset exprExportTargetInstanceId and exprExportCategory.
696-697:⚠️ Potential issue | 🟡 MinorHandle target metadata errors separately from “no categories.”
Only
targetMetadataLoadingis checked here. IfuseInstanceMetadata(...)errors,targetCategoriesis also empty, so the UI falls back to “No categories found on target” even though categories were never loaded. That hides transient failures as a successful empty state; render an explicit error branch and reserve the manual-entry fallback for a successful load with zero categories.Also applies to: 3461-3500
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/instances/preferences/WorkflowDialog.tsx` around lines 696 - 697, The UI currently only checks targetMetadataLoading so when useInstanceMetadata(...) returns an error the code treats targetCategories as empty and shows “No categories found on target”; update the render logic around useInstanceMetadata/targetMetadata/targetMetadataLoading to also check the hook's error state (e.g., isError or error) and render an explicit error branch when target metadata failed to load, reserving the manual-entry “no categories” fallback only when !targetMetadataLoading && !targetMetadataError && targetCategories.length === 0; apply the same change to the duplicate block that references useInstanceMetadata/targetMetadata/targetCategories around the other indicated region.internal/services/automations/service.go (3)
3865-3892:⚠️ Potential issue | 🟠 MajorExport-only timeout/cancel paths can still suppress notifications.
mergePartial()only folds in activities that already finished. If none completed before the drain is interrupted,summarystays empty andnotifyAutomationSummary()returns early, so export-only rules still produce no notification in this path.Also applies to: 3896-3902
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/automations/service.go` around lines 3865 - 3892, mergePartial currently only folds finished activities so when the drain is interrupted and no activities completed the summary stays empty and notifyAutomationSummary short-circuits; change the control flow so notifyAutomationSummary is always invoked for interrupted drain paths: in mergePartial (and the select branches that call it) set a flag on the surrounding scope (e.g., partialDrainOccurred bool) or mark the summary (e.g., summary.MarkPartial()) to indicate an interrupted export, and after the select ensure you call notifyAutomationSummary(notifyCtx, instanceID, summary, eligibleRules) even if summary is empty (or synthesize a minimal summary/marker) so export-only rules in eligibleRules still produce a notification; keep the existing notifyCtx creation (ctx / context.WithTimeout(..., s.cfg.ApplyTimeout)) and pass that to notifyAutomationSummary.
6072-6073:⚠️ Potential issue | 🟠 MajorUse a fresh bounded context for the recovery recheck and target cleanup.
On timeout/cancel paths,
verifyExportOnTarget()can return withctxalready done, and the recheck plusBulkAction("delete")then run on that same expired context. That leaves a stale torrent on the target, and the next pass will treat the export as already satisfied instead of retrying.Also applies to: 6184-6203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/automations/service.go` around lines 6072 - 6073, The recovery recheck and target cleanup are running on the same context used for verifyExportOnTarget(), which may be already cancelled; update the code so that after verifyExportOnTarget() returns you create and use a fresh bounded context (e.g., ctx, cancel := context.WithTimeout(context.Background(), exportTimeout); defer cancel()) for the subsequent recheck and any BulkAction("delete") cleanup to ensure those operations have a live context; apply the same change around the other similar block referenced (the section covering verifyExportOnTarget() and BulkAction("delete") at the later occurrence).
2617-2631:⚠️ Potential issue | 🟠 MajorStart the per-instance timeout before export preflight.
These target lookups are still running on the outer
ctxuntil Line 2720. On a large export set, this can spendN * 10shere and blow pastApplyTimeoutbefore any action execution starts, which defeats the config's documented scope.Also applies to: 2720-2721
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/automations/service.go` around lines 2617 - 2631, The per-instance 10s timeout must be started before any target lookup or preflight so the HasTorrentByAnyHash call runs under the bounded context; change the code that builds pendingExportToInstance/state.exportToInstance handling so you create a per-instance context via context.WithTimeout(ctx, 10*time.Second) and use that checkCtx when calling s.syncManager.HasTorrentByAnyHash (and any other per-target preflight like the later lookup at lines ~2720-2721) — ensure you call the corresponding cancel() immediately after the HasTorrentByAnyHash call; reference state.exportToInstance, pendingExportToInstance and syncManager.HasTorrentByAnyHash to locate and update the code paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/services/automations/service.go`:
- Around line 3865-3892: mergePartial currently only folds finished activities
so when the drain is interrupted and no activities completed the summary stays
empty and notifyAutomationSummary short-circuits; change the control flow so
notifyAutomationSummary is always invoked for interrupted drain paths: in
mergePartial (and the select branches that call it) set a flag on the
surrounding scope (e.g., partialDrainOccurred bool) or mark the summary (e.g.,
summary.MarkPartial()) to indicate an interrupted export, and after the select
ensure you call notifyAutomationSummary(notifyCtx, instanceID, summary,
eligibleRules) even if summary is empty (or synthesize a minimal summary/marker)
so export-only rules in eligibleRules still produce a notification; keep the
existing notifyCtx creation (ctx / context.WithTimeout(..., s.cfg.ApplyTimeout))
and pass that to notifyAutomationSummary.
- Around line 6072-6073: The recovery recheck and target cleanup are running on
the same context used for verifyExportOnTarget(), which may be already
cancelled; update the code so that after verifyExportOnTarget() returns you
create and use a fresh bounded context (e.g., ctx, cancel :=
context.WithTimeout(context.Background(), exportTimeout); defer cancel()) for
the subsequent recheck and any BulkAction("delete") cleanup to ensure those
operations have a live context; apply the same change around the other similar
block referenced (the section covering verifyExportOnTarget() and
BulkAction("delete") at the later occurrence).
- Around line 2617-2631: The per-instance 10s timeout must be started before any
target lookup or preflight so the HasTorrentByAnyHash call runs under the
bounded context; change the code that builds
pendingExportToInstance/state.exportToInstance handling so you create a
per-instance context via context.WithTimeout(ctx, 10*time.Second) and use that
checkCtx when calling s.syncManager.HasTorrentByAnyHash (and any other
per-target preflight like the later lookup at lines ~2720-2721) — ensure you
call the corresponding cancel() immediately after the HasTorrentByAnyHash call;
reference state.exportToInstance, pendingExportToInstance and
syncManager.HasTorrentByAnyHash to locate and update the code paths.
In `@web/src/components/instances/preferences/WorkflowDialog.tsx`:
- Around line 3423-3442: The select for export target can hide a currently set
exprExportTargetInstanceId when its instance was deleted/filtered, leaving the
form appearing empty; update the render logic in the WorkflowDialog component
around the Select (where nonSelfInstances is mapped to SelectItem and value uses
formState.exprExportTargetInstanceId) to handle missing/unknown IDs at render
time: check whether formState.exprExportTargetInstanceId exists in
nonSelfInstances (by id), and if not either render a disabled SelectItem labeled
like "Missing instance (id: ...)" with value set to the hydrated id or normalize
the value to null before passing into the SelectValue; do this in the render
path (not via useEffect) and use setFormState only when user changes selection
via onValueChange to clear/reset exprExportTargetInstanceId and
exprExportCategory.
- Around line 696-697: The UI currently only checks targetMetadataLoading so
when useInstanceMetadata(...) returns an error the code treats targetCategories
as empty and shows “No categories found on target”; update the render logic
around useInstanceMetadata/targetMetadata/targetMetadataLoading to also check
the hook's error state (e.g., isError or error) and render an explicit error
branch when target metadata failed to load, reserving the manual-entry “no
categories” fallback only when !targetMetadataLoading && !targetMetadataError &&
targetCategories.length === 0; apply the same change to the duplicate block that
references useInstanceMetadata/targetMetadata/targetCategories around the other
indicated region.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ed37eae2-5dd7-4181-bcc7-0aa60e911825
📒 Files selected for processing (4)
internal/api/handlers/automations.gointernal/services/automations/service.goweb/src/components/instances/preferences/WorkflowDialog.tsxweb/src/components/instances/preferences/WorkflowsOverview.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/api/handlers/automations.go
- web/src/components/instances/preferences/WorkflowsOverview.tsx
Document the new Export to Instance automation action including all configuration options, save path templates, behavior notes, and processing order.
nitrobass24
added
automations
area/backend
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.
Summary
.torrentfile from one qBittorrent instance and adds it to another, enabling automated torrent migration (e.g., seedbox → local instance for long-term seeding)skip_checking=trueby defaultAction Options
{{ .Name }},{{ .Category }},{{ .Hash }},{{ .Tracker }})Changes
ExportToInstanceActionstruct, validation, activity constantExportTorrent+AddTorrent, activity logging, dry-run supportRelated
Test plan
make build— backend + frontend compile cleanmake test— all existing tests pass (updatedrecordDryRunActivitiescall sites)Summary by CodeRabbit
New Features
UI
Validation