feat(cross-seed): add "by-tracker" directory preset with per-indexer category mapping#1783
Open
CjE5 wants to merge 2 commits into
Open
feat(cross-seed): add "by-tracker" directory preset with per-indexer category mapping#1783CjE5 wants to merge 2 commits into
CjE5 wants to merge 2 commits into
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 per-instance cross-seed indexer→category mappings: DB migrations, a new store and model, announce-domain↔indexer matching and aliasing, service and dirscan integration for tracker-category mode, new API/proxy endpoints plus frontend UI, and tests across backend and frontend. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as HTTP API
participant Handler as CrossSeedHandler
participant Store as CrossSeedIndexerCategoryStore
participant DB as Database
Client->>API: GET /api/cross-seed/indexer-categories/{instanceId}
API->>Handler: GetInstanceIndexerCategories(w,r)
Handler->>Handler: parse & validate instanceId
Handler->>Store: List(ctx, instanceId)
Store->>DB: SELECT ... JOIN ...
DB-->>Store: rows
Store-->>Handler: []CrossSeedIndexerCategory
Handler-->>Client: 200 + JSON
sequenceDiagram
participant Dirscan as Dirscan Service
participant Crossseed as CrossSeed Service
participant Store as CrossSeedIndexerCategoryStore
participant Qbit as qBittorrent
participant Disk as Host FS
Dirscan->>Crossseed: ResolveTrackerCategory(ctx, instanceId, indexerName, announceDomain)
Crossseed->>Store: List/GetByIndexerName(ctx, instanceId, indexerName)
Store-->>Crossseed: category (or empty)
alt category found
Crossseed->>Qbit: GetCategories()
Qbit-->>Crossseed: categories map (contains SavePath)
Crossseed-->>Dirscan: (category, savePath)
Dirscan->>Disk: mapTrackerCategorySavePathToHost(savePath)
Disk-->>Dirscan: host path
Dirscan->>Dirscan: Inject with TrackerCategorySavePath (autoTMM=true)
else not found
Crossseed-->>Dirscan: ("", "")
Dirscan->>Dirscan: fallback injection path
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/services/crossseed/service.go (2)
11214-11223:⚠️ Potential issue | 🟠 MajorDisable AutoTMM when category creation fails.
placement.EnableAutoTMMis computed beforeensureCrossCategory. If category creation fails,crossCategoryis cleared, but the later add options can still sendautoTMM=truewith no category and no explicitsavepath, so qBittorrent can place the torrent in its default managed location instead ofplan.RootDir.🐛 Proposed fix
- if placement.EnableAutoTMM { + if placement.EnableAutoTMM && crossCategory != "" { options["autoTMM"] = "true" } else { options["autoTMM"] = "false" options["savepath"] = plan.RootDir }Apply the same guard in both
processHardlinkModeandprocessReflinkMode.Also applies to: 11328-11333, 11841-11849, 11944-11949
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/crossseed/service.go` around lines 11214 - 11223, The code computes placement.EnableAutoTMM before calling s.ensureCrossCategory and clears crossCategory on failure, but never turns off AutoTMM, allowing qBittorrent to auto-manage placement; update the logic in processHardlinkMode and processReflinkMode (and the analogous locations noted) so that if ensureCrossCategory returns an error (categoryCreationFailed true) you set placement.EnableAutoTMM = false (or otherwise disable AutoTMM on the placement/options object you later pass to AddTorrent/AddTorrentFromFile), ensuring no auto-TMM is sent when crossCategory was cleared; locate uses of ensureCrossCategory, placement.EnableAutoTMM, and the add-option construction in those functions and apply the same guard in all listed blocks.
11750-11804:⚠️ Potential issue | 🟠 MajorUse the tracker category save path as the reflink base when available.
Hardlink mode already validates and uses
categorySavePathdirectly, but reflink mode still requiresinstance.HardlinkBaseDirand checks reflink support on that directory beforeresolveLinkModeCategoryPlacementlater chooses the category path asdestDir. A by-tracker category with a valid configured save path can therefore be rejected becauseHardlinkBaseDiris empty/wrong, or the wrong filesystem can be validated before cloning intocategorySavePath.🐛 Proposed fix
- // Validate base directory is configured (reuses hardlink base dir) - if instance.HardlinkBaseDir == "" { - log.Warn(). - Int("instanceID", candidate.InstanceID). - Msg("[CROSSSEED] Reflink mode enabled but base directory is empty") - return handleError("Reflink mode enabled but base directory is not configured") - } - // Verify instance has local filesystem access (required for reflinks) if !instance.HasLocalFilesystemAccess { log.Warn(). @@ - selectedBaseDir, err := FindMatchingBaseDir(instance.HardlinkBaseDir, existingFilePath) - if err != nil { - log.Warn(). - Err(err). - Str("configuredDirs", instance.HardlinkBaseDir). - Str("existingPath", existingFilePath). - Msg("[CROSSSEED] Reflink mode: no suitable base directory found") - return handleError(fmt.Sprintf("No suitable base directory: %v", err)) - } + var selectedBaseDir string + if categorySavePath != "" { + if err := os.MkdirAll(categorySavePath, 0o755); err != nil { + return handleError(fmt.Sprintf("Failed to create category directory %q: %v", categorySavePath, err)) + } + sameFS, err := fsutil.SameFilesystem(existingFilePath, categorySavePath) + if err != nil { + return handleError(fmt.Sprintf("Failed to check filesystem for category path %q: %v", categorySavePath, err)) + } + if !sameFS { + return handleError(fmt.Sprintf("Category save path %q is not on the same filesystem as source files — reflinks require compatible source/destination filesystems", categorySavePath)) + } + selectedBaseDir = categorySavePath + } else { + if instance.HardlinkBaseDir == "" { + log.Warn(). + Int("instanceID", candidate.InstanceID). + Msg("[CROSSSEED] Reflink mode enabled but base directory is empty") + return handleError("Reflink mode enabled but base directory is not configured") + } + + selectedBaseDir, err = FindMatchingBaseDir(instance.HardlinkBaseDir, existingFilePath) + if err != nil { + log.Warn(). + Err(err). + Str("configuredDirs", instance.HardlinkBaseDir). + Str("existingPath", existingFilePath). + Msg("[CROSSSEED] Reflink mode: no suitable base directory found") + return handleError(fmt.Sprintf("No suitable base directory: %v", err)) + } + } // Check reflink support supported, reason := reflinktree.SupportsReflink(selectedBaseDir)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/crossseed/service.go` around lines 11750 - 11804, The reflink validation should prefer the tracker category's save path when present: when building existingFilePath and selecting the base for reflink checks, if a non-empty categorySavePath (the tracker category save path used later by resolveLinkModeCategoryPlacement as destDir) is available use that as the base to call reflinktree.SupportsReflink and FindMatchingBaseDir; otherwise fall back to instance.HardlinkBaseDir as before. Update the logic around matchedTorrent.ContentPath/props.SavePath and the selectedBaseDir selection (where FindMatchingBaseDir is called and reflinktree.SupportsReflink is checked) to try categorySavePath first, then instance.HardlinkBaseDir, and keep existing warnings/handleError calls when neither yields a valid base or reflinks unsupported.
🧹 Nitpick comments (5)
internal/database/migrations/071_add_tracker_category_settings.sql (1)
15-15: Nit:DEFAULT ''oncategoryis unreachable.
CrossSeedIndexerCategoryStore.Settrims and rejects blank categories before inserting, and it's the only writer on this table. The default value can never be stored, soDEFAULT ''is dead; consider dropping it (and the companion default on the Postgres migration) to avoid confusion about whether blank mappings are valid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/database/migrations/071_add_tracker_category_settings.sql` at line 15, The migration currently defines the column "category TEXT NOT NULL DEFAULT ''", but CrossSeedIndexerCategoryStore.Set trims and rejects blank categories so the empty-string default is never used; remove the DEFAULT '' from the column definition in this migration (and remove the matching DEFAULT '' in the Postgres migration counterpart) so the schema doesn't imply blank categories are valid—keep NOT NULL but drop the default to avoid confusion with CrossSeedIndexerCategoryStore.Set behavior.internal/proxy/crossseed_indexer_categories_test.go (1)
81-163: Consider table-driving these handler cases.The status/error/success tests repeat the same request, recorder, handler call, and decode flow. A small table with per-case setup would better match the repo test style and make adding more proxy endpoint cases cheaper.
♻️ Possible shape
+func TestHandleCrossSeedIndexerCategories(t *testing.T) { + tests := []struct { + name string + setup func(t *testing.T) (*Handler, int) + wantStatus int + wantErr bool + wantMappings int + }{ + // missing instance, nil store, empty slice, populated mapping... + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + h, instanceID := tc.setup(t) + req := httptest.NewRequestWithContext(context.Background(), http.MethodGet, "/api/v2/cross-seed/indexer-categories", nil) + req = req.WithContext(context.WithValue(req.Context(), InstanceIDContextKey, instanceID)) + + rec := httptest.NewRecorder() + h.handleCrossSeedIndexerCategories(rec, req) + + require.Equal(t, tc.wantStatus, rec.Code) + // decode error or mappings based on tc.wantErr + }) + } +}As per coding guidelines,
**/*_test.go: “Use table-driven test cases and reuse integration fixtures frominternal/qbittorrent/”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/proxy/crossseed_indexer_categories_test.go` around lines 81 - 163, The tests for handleCrossSeedIndexerCategories are repetitive—refactor TestHandleCrossSeedIndexerCategories_* into a single table-driven test that iterates cases (e.g., "missing instance id", "nil store", "empty slice", "returns mappings"); for each case define a name, optional store setup (use setupIndexerCategoryProxyDB, models.NewCrossSeedIndexerCategoryStore, insertProxyTestInstance, insertProxyTestIndexer, store.Set where needed), the context InstanceID value, expected HTTP status and a post-check function for the decoded body; inside the loop create the request, attach context, create httptest.NewRecorder(), call h.handleCrossSeedIndexerCategories, assert status and Content-Type, then decode and run the per-case assertions—this consolidates repeated request/recorder/handler call/decode logic while keeping existing helpers and the handleCrossSeedIndexerCategories target unchanged.internal/models/crossseed_indexer_categories_test.go (1)
160-214: Consider consolidating the repeated lookup cases into table-driven tests.These cases share the same setup/assertion shape and would be easier to extend as
[]struct{name, input, want string}witht.Run. As per coding guidelines,**/*_test.go: “Use table-driven test cases and reuse integration fixtures frominternal/qbittorrent/”.Also applies to: 235-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/models/crossseed_indexer_categories_test.go` around lines 160 - 214, Consolidate the repeated GetByIndexerName test cases into table-driven subtests: replace the multiple functions TestCrossSeedIndexerCategoryStore_GetByIndexerName, TestCrossSeedIndexerCategoryStore_GetByIndexerName_CaseInsensitive, TestCrossSeedIndexerCategoryStore_GetByIndexerName_NotFound, and TestCrossSeedIndexerCategoryStore_GetByIndexerName_EmptyName with a single TestCrossSeedIndexerCategoryStore_GetByIndexerName that defines a []struct{name string; instanceID int; setup bool; input string; want string} (or similar) and iterates with t.Run; reuse the common setup logic (db := setupIndexerCategoryTestDB, store := models.NewCrossSeedIndexerCategoryStore, ctx := context.Background()) and call store.Set only for rows that need pre-inserted data (reference insertTestInstance, insertTestTorznabIndexer, and store.Set), then assert results for each case (expecting empty string or "Aither") to cover case-insensitive, not-found and empty-name scenarios.internal/services/dirscan/service.go (1)
1786-1825: Extract the by-tracker resolution branch fromtryMatchAndInject.This branch is correct, but it adds another responsibility to an already long match/download/inject function. A small helper returning
(category, trackerCategorySavePath string, failed *searcheeMatch)would keep the injection path easier to scan. As per coding guidelines, Go code is checked with golangci-lint v2 using “funlen function length threshold at 80 lines” and “gocognit cognitive complexity threshold at 15”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/dirscan/service.go` around lines 1786 - 1825, Extract the "by-tracker" resolution branch out of tryMatchAndInject into a small helper function (e.g., resolveByTrackerCategory) that returns (category string, trackerCategorySavePath string, failed *searcheeMatch); move the logic that checks s.indexerCategoryStore, loads the instance via s.instanceStore.Get, calls resolveDirscanTrackerCategory, and builds the failMatch searcheeMatch into that helper, preserving the same parameters used here (ctx, dir.TargetInstanceID, result.Indexer, announceDomain, instance, s.indexerCategoryStore, s.syncManager, l) and the same failure/fatal handling; update tryMatchAndInject to call the new helper, assign returned category and trackerCategorySavePath if non-empty, and if failed != nil return it, ensuring behavior is identical but reducing tryMatchAndInject's length and cognitive complexity.internal/services/dirscan/inject.go (1)
670-762: Consider extracting tracker-category destination resolution frommaterializeLinkTree.
materializeLinkTreenow spans roughly 90+ lines and mixes base-dir mapping, destination selection, plan diagnostics, and link creation. Extracting the tracker/non-tracker destination selection into a helper would keep this under the repo’s lint threshold and make the new branch easier to test. As per coding guidelines, “Use golangci-lint v2 with strict configuration targeting AI-generated code patterns, with funlen function length threshold at 80 lines”.Also applies to: 801-810
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/dirscan/inject.go` around lines 670 - 762, The materializeLinkTree function mixes tracker-category mapping, base-dir selection, and destDir creation making it too long; extract the tracker vs non-tracker destination resolution into a helper (e.g., resolveLinkDestination or selectLinkDestination) that accepts (ctx, instance, req, incomingFiles/existingFiles) and returns (selectedBaseDir, destDir, error), encapsulating the mapTrackerCategorySavePathToHost call, the crossseed.FindMatchingBaseDir call, the os.MkdirAll calls, and the buildLinkDestDir invocation; update materializeLinkTree to call this helper and use its returned selectedBaseDir and destDir before calling hardlinktree.BuildPlan so the main function is shortened and easier to test.
🤖 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/crossseed/auto_tmm_test.go`:
- Around line 170-181: The test case named "tracker category mode with
mismatched paths - auto tmm disabled" is asserting AutoTMM is disabled even
though the mapped tracker category has a configured save path; update the
expectation to match by-tracker AutoTMM semantics by setting wantEnabled to true
(AutoTMM enabled) while keeping wantPathsMatch false, and ensure the test still
uses isTrackerCategoryMode, actualCategorySavePath, and matchedSavePath to
reflect the mismatched paths scenario.
In `@internal/services/crossseed/service.go`:
- Around line 10111-10118: The ResolveTrackerCategory call is treating a
store/context error (resolveErr) as “no mapping” and falling back to other
category rules; instead surface resolveErr to the caller so the inject can
fail/retry. Change the branch in the block around ResolveTrackerCategory (the
variables cat, found, resolveErr and s.indexerCategoryStore) to return or
propagate resolveErr to processCrossSeedCandidate rather than logging and
continuing, and update processCrossSeedCandidate’s caller chain to handle/return
that error appropriately so mapping-store failures are not treated as “no
mapping.”
In `@internal/services/crossseed/tracker_category.go`:
- Around line 93-99: The current containment fallback in tracker_category.go
(checking strings.Contains between normalizedIndexer and domainNorm)
reintroduces false positives (e.g., "notbeyondhd" containing "beyondhd");
replace this loose substring check with a boundary-aware match: only treat a
match as true if normalizedIndexer appears in domainNorm with non-alphanumeric
boundaries (or at the string edges) — e.g., use a regex or explicit checks for
separators/edges around the match — and remove the plain strings.Contains
fallback; for domains that truly need non-textual mapping, add explicit entries
in internal/services/crossseed/domain_aliases.go instead of relying on substring
containment.
---
Outside diff comments:
In `@internal/services/crossseed/service.go`:
- Around line 11214-11223: The code computes placement.EnableAutoTMM before
calling s.ensureCrossCategory and clears crossCategory on failure, but never
turns off AutoTMM, allowing qBittorrent to auto-manage placement; update the
logic in processHardlinkMode and processReflinkMode (and the analogous locations
noted) so that if ensureCrossCategory returns an error (categoryCreationFailed
true) you set placement.EnableAutoTMM = false (or otherwise disable AutoTMM on
the placement/options object you later pass to AddTorrent/AddTorrentFromFile),
ensuring no auto-TMM is sent when crossCategory was cleared; locate uses of
ensureCrossCategory, placement.EnableAutoTMM, and the add-option construction in
those functions and apply the same guard in all listed blocks.
- Around line 11750-11804: The reflink validation should prefer the tracker
category's save path when present: when building existingFilePath and selecting
the base for reflink checks, if a non-empty categorySavePath (the tracker
category save path used later by resolveLinkModeCategoryPlacement as destDir) is
available use that as the base to call reflinktree.SupportsReflink and
FindMatchingBaseDir; otherwise fall back to instance.HardlinkBaseDir as before.
Update the logic around matchedTorrent.ContentPath/props.SavePath and the
selectedBaseDir selection (where FindMatchingBaseDir is called and
reflinktree.SupportsReflink is checked) to try categorySavePath first, then
instance.HardlinkBaseDir, and keep existing warnings/handleError calls when
neither yields a valid base or reflinks unsupported.
---
Nitpick comments:
In `@internal/database/migrations/071_add_tracker_category_settings.sql`:
- Line 15: The migration currently defines the column "category TEXT NOT NULL
DEFAULT ''", but CrossSeedIndexerCategoryStore.Set trims and rejects blank
categories so the empty-string default is never used; remove the DEFAULT '' from
the column definition in this migration (and remove the matching DEFAULT '' in
the Postgres migration counterpart) so the schema doesn't imply blank categories
are valid—keep NOT NULL but drop the default to avoid confusion with
CrossSeedIndexerCategoryStore.Set behavior.
In `@internal/models/crossseed_indexer_categories_test.go`:
- Around line 160-214: Consolidate the repeated GetByIndexerName test cases into
table-driven subtests: replace the multiple functions
TestCrossSeedIndexerCategoryStore_GetByIndexerName,
TestCrossSeedIndexerCategoryStore_GetByIndexerName_CaseInsensitive,
TestCrossSeedIndexerCategoryStore_GetByIndexerName_NotFound, and
TestCrossSeedIndexerCategoryStore_GetByIndexerName_EmptyName with a single
TestCrossSeedIndexerCategoryStore_GetByIndexerName that defines a []struct{name
string; instanceID int; setup bool; input string; want string} (or similar) and
iterates with t.Run; reuse the common setup logic (db :=
setupIndexerCategoryTestDB, store := models.NewCrossSeedIndexerCategoryStore,
ctx := context.Background()) and call store.Set only for rows that need
pre-inserted data (reference insertTestInstance, insertTestTorznabIndexer, and
store.Set), then assert results for each case (expecting empty string or
"Aither") to cover case-insensitive, not-found and empty-name scenarios.
In `@internal/proxy/crossseed_indexer_categories_test.go`:
- Around line 81-163: The tests for handleCrossSeedIndexerCategories are
repetitive—refactor TestHandleCrossSeedIndexerCategories_* into a single
table-driven test that iterates cases (e.g., "missing instance id", "nil store",
"empty slice", "returns mappings"); for each case define a name, optional store
setup (use setupIndexerCategoryProxyDB, models.NewCrossSeedIndexerCategoryStore,
insertProxyTestInstance, insertProxyTestIndexer, store.Set where needed), the
context InstanceID value, expected HTTP status and a post-check function for the
decoded body; inside the loop create the request, attach context, create
httptest.NewRecorder(), call h.handleCrossSeedIndexerCategories, assert status
and Content-Type, then decode and run the per-case assertions—this consolidates
repeated request/recorder/handler call/decode logic while keeping existing
helpers and the handleCrossSeedIndexerCategories target unchanged.
In `@internal/services/dirscan/inject.go`:
- Around line 670-762: The materializeLinkTree function mixes tracker-category
mapping, base-dir selection, and destDir creation making it too long; extract
the tracker vs non-tracker destination resolution into a helper (e.g.,
resolveLinkDestination or selectLinkDestination) that accepts (ctx, instance,
req, incomingFiles/existingFiles) and returns (selectedBaseDir, destDir, error),
encapsulating the mapTrackerCategorySavePathToHost call, the
crossseed.FindMatchingBaseDir call, the os.MkdirAll calls, and the
buildLinkDestDir invocation; update materializeLinkTree to call this helper and
use its returned selectedBaseDir and destDir before calling
hardlinktree.BuildPlan so the main function is shortened and easier to test.
In `@internal/services/dirscan/service.go`:
- Around line 1786-1825: Extract the "by-tracker" resolution branch out of
tryMatchAndInject into a small helper function (e.g., resolveByTrackerCategory)
that returns (category string, trackerCategorySavePath string, failed
*searcheeMatch); move the logic that checks s.indexerCategoryStore, loads the
instance via s.instanceStore.Get, calls resolveDirscanTrackerCategory, and
builds the failMatch searcheeMatch into that helper, preserving the same
parameters used here (ctx, dir.TargetInstanceID, result.Indexer, announceDomain,
instance, s.indexerCategoryStore, s.syncManager, l) and the same failure/fatal
handling; update tryMatchAndInject to call the new helper, assign returned
category and trackerCategorySavePath if non-empty, and if failed != nil return
it, ensuring behavior is identical but reducing tryMatchAndInject's length and
cognitive complexity.
🪄 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: b7633239-19f9-4929-aa75-995079e1fc83
📒 Files selected for processing (34)
cmd/qui/main.gointernal/api/handlers/crossseed.gointernal/api/handlers/dirscan_webhook_test.gointernal/api/server.gointernal/api/server_test.gointernal/database/migrations/071_add_tracker_category_settings.sqlinternal/database/postgres_migrations/072_add_tracker_category_settings.sqlinternal/models/crossseed_indexer_categories.gointernal/models/crossseed_indexer_categories_test.gointernal/proxy/crossseed_indexer_categories_test.gointernal/proxy/handler.gointernal/proxy/handler_test.gointernal/services/crossseed/auto_tmm_test.gointernal/services/crossseed/category_lookup_failure_test.gointernal/services/crossseed/category_persist_path_test.gointernal/services/crossseed/crossseed_test.gointernal/services/crossseed/domain_aliases.gointernal/services/crossseed/domain_aliases_test.gointernal/services/crossseed/domain_matching_test.gointernal/services/crossseed/hardlink_mode_test.gointernal/services/crossseed/link_mode_category_placement_test.gointernal/services/crossseed/service.gointernal/services/crossseed/tracker_category.gointernal/services/crossseed/tracker_category_test.gointernal/services/dirscan/cancel_scan_test.gointernal/services/dirscan/inject.gointernal/services/dirscan/inject_test.gointernal/services/dirscan/service.gointernal/services/dirscan/service_tracker_category_test.gointernal/services/dirscan/webhook_queue_test.gointernal/web/swagger/openapi.yamlweb/src/lib/api.tsweb/src/pages/CrossSeedPage.tsxweb/src/types/index.ts
Comment on lines
+170
to
+181
| // AutoTMM must NOT be enabled when paths differ: qBittorrent would move the | ||
| // existing files to the category directory instead of cross-seeding in place. | ||
| name: "tracker category mode with mismatched paths - auto tmm disabled", | ||
| crossCategory: "Aither", | ||
| matchedAutoManaged: false, | ||
| useCategoryFromIndexer: false, | ||
| useCustomCategory: false, | ||
| isTrackerCategoryMode: true, | ||
| actualCategorySavePath: "/downloads/aither", | ||
| matchedSavePath: "/downloads/tv", | ||
| wantEnabled: false, | ||
| wantPathsMatch: false, |
Contributor
There was a problem hiding this comment.
Recheck this expectation against by-tracker AutoTMM semantics.
This case disables AutoTMM even though the mapped category has a configured save path. That appears to contradict the PR objective: by-tracker mappings should enable AutoTMM when qBittorrent has a save path for the mapped category, even when that path differs from the matched torrent’s current save path.
If the category path is configured, the expected decision should likely keep AutoTMM enabled while PathsMatch remains false.
Proposed test expectation adjustment
- name: "tracker category mode with mismatched paths - auto tmm disabled",
+ name: "tracker category mode with configured category path - auto tmm enabled",
crossCategory: "Aither",
matchedAutoManaged: false,
useCategoryFromIndexer: false,
useCustomCategory: false,
isTrackerCategoryMode: true,
actualCategorySavePath: "/downloads/aither",
matchedSavePath: "/downloads/tv",
- wantEnabled: false,
+ wantEnabled: true,
wantPathsMatch: false,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // AutoTMM must NOT be enabled when paths differ: qBittorrent would move the | |
| // existing files to the category directory instead of cross-seeding in place. | |
| name: "tracker category mode with mismatched paths - auto tmm disabled", | |
| crossCategory: "Aither", | |
| matchedAutoManaged: false, | |
| useCategoryFromIndexer: false, | |
| useCustomCategory: false, | |
| isTrackerCategoryMode: true, | |
| actualCategorySavePath: "/downloads/aither", | |
| matchedSavePath: "/downloads/tv", | |
| wantEnabled: false, | |
| wantPathsMatch: false, | |
| name: "tracker category mode with configured category path - auto tmm enabled", | |
| crossCategory: "Aither", | |
| matchedAutoManaged: false, | |
| useCategoryFromIndexer: false, | |
| useCustomCategory: false, | |
| isTrackerCategoryMode: true, | |
| actualCategorySavePath: "/downloads/aither", | |
| matchedSavePath: "/downloads/tv", | |
| wantEnabled: true, | |
| wantPathsMatch: false, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/services/crossseed/auto_tmm_test.go` around lines 170 - 181, The
test case named "tracker category mode with mismatched paths - auto tmm
disabled" is asserting AutoTMM is disabled even though the mapped tracker
category has a configured save path; update the expectation to match by-tracker
AutoTMM semantics by setting wantEnabled to true (AutoTMM enabled) while keeping
wantPathsMatch false, and ensure the test still uses isTrackerCategoryMode,
actualCategorySavePath, and matchedSavePath to reflect the mismatched paths
scenario.
Collaborator
But part from that (hopefully without sounding too critical), I'm not really sure anymore if the feature brings any real usefulness? What does it bring that isnt already solved by these? 
|
Contributor
Author
|
Hey @s0up4200, totally fair question - happy to clarify. The goal of this feature is to have all cross-seeds for a given tracker land in a single, consistent directory regardless of which cross-seed app injected them, with a user-defined qBittorrent category assigned and AutoTMM enabled - so files can be easily moved later as needed without breaking anything. Affix vs. mapping: Category affix transforms whatever category cross-seed would have resolved into a derived name (e.g. BeyondHD → BeyondHD.cs). The intent is isolation - marking cross-seeds as distinct. My goal is the opposite: I want cross-seeds for a given tracker to land in the same category as regular downloads for that tracker (BeyondHD → hd-movies), no duplication. Affix has no way to express that. AutoTMM + hardlinks: When a category has a configured save path in qBittorrent, this feature looks up that path, pre-places the hardlinks directly under it, and then enables AutoTMM on the added torrent. Without pre-placement, enabling AutoTMM would cause qBittorrent to move the files to the category directory - breaking the hardlinks. No existing mechanism handles this sequence. Centralization: I run more than one cross-seed app. This feature makes qui the authority on tracker categories and save paths - the mapping lives in qui's database and is exposed via a proxy-authenticated endpoint. Any cross-seed type of app can query it to resolve the correct category without each needing its own configuration. qui's by-tracker is orthogonal: That setting controls where hardlinks land on the host filesystem - tracker-named subfolders for multi-file torrents, auto-generated isolation folders (TorrentName--<hash>) for single-file torrents. These paths are not customizable and result in inconsistencies across apps that don't share the same hardlink base directory layout. And, ultimately, it says nothing about which qBittorrent category is assigned or whether AutoTMM is enabled - the two features operate at different layers. I'd also love to understand your perspective better - is there a specific combination of existing features you think would achieve the same result? If there's an approach I'm missing I'm genuinely open to it, but from what I can tell none of the existing mechanisms get you consistent single-directory placement per tracker across multiple apps with AutoTMM wired up correctly. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/services/crossseed/hardlink_mode_test.go (1)
950-994: Keep the regression test from touching absolute host paths.If this guard regresses, the test may attempt
MkdirAll("/qbt/movies"); in privileged CI that can mutate the host filesystem instead of failing safely. Use a temp-file blocker so the category-path branch deterministically fails without touching/.🧪 Proposed test-hardening diff
func TestProcessHardlinkMode_IgnoresCategorySavePathWhenNotTrackerCategoryMode(t *testing.T) { mockInstances := &mockInstanceStore{ instances: map[int]*models.Instance{ @@ s := &Service{instanceStore: mockInstances} + categoryPathBlocker := filepath.Join(t.TempDir(), "category-save-path-blocker") + require.NoError(t, os.WriteFile(categoryPathBlocker, []byte("not a directory"), 0o600)) + categorySavePath := filepath.Join(categoryPathBlocker, "movies") + result := s.processHardlinkMode( context.Background(), CrossSeedCandidate{InstanceID: 1, InstanceName: "qbt1"}, @@ &qbt.TorrentProperties{SavePath: "/downloads"}, "movies", "movies.cross", - "/qbt/movies", // categorySavePath from matched torrent's existing category + categorySavePath, // categorySavePath from matched torrent's existing category false, // isTrackerCategoryMode = false: no mapping configured true, // crossCategoryExistsInQbit = true: category already in qBittorrent )As per coding guidelines, when adding Go tests that create files with
os.WriteFile, use0o600or tighter permissions unless broader mode bits are explicitly needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/crossseed/hardlink_mode_test.go` around lines 950 - 994, The test TestProcessHardlinkMode_IgnoresCategorySavePathWhenNotTrackerCategoryMode can accidentally call os.MkdirAll on an absolute host path ("/qbt/movies") if the guard around isTrackerCategoryMode regresses; modify the test to create a temporary blocker file/path (e.g., via os.CreateTemp or os.WriteFile) at a safe temp location and point categorySavePath to that temp path so the category-path branch deterministically fails without touching root, and ensure any temp files are created with tight permissions (0o600) and cleaned up; verify the assertion still checks that the error came from FindMatchingBaseDir and not the category directory branch, and reference processHardlinkMode, FindMatchingBaseDir, MkdirAll, categorySavePath and isTrackerCategoryMode when locating the code to change.internal/services/crossseed/link_mode_category_placement_test.go (1)
26-53: Add coverage for an existing mapped category with no save path.
resolveLinkModeCategoryPlacementhas a distinct branch forisTrackerCategoryMode=true,crossCategoryExistsInQbit=true, and emptycategorySavePath; this should assert AutoTMM stays disabled and placement falls back to preset-derived destination.🧪 Proposed additional table case
{ name: "existing tracker category uses configured save path directly", candidateFiles: filesRootless, categorySavePath: "/qbt/Aither", crossCategoryExistsInQbt: true, wantSavePath: "/qbt/Aither", wantDestDir: "/qbt/Aither", wantAutoTMM: true, }, + { + name: "existing tracker category without configured save path disables AutoTMM", + candidateFiles: filesWithCommonRoot, + categorySavePath: "", + crossCategoryExistsInQbt: true, + wantSavePath: "", + wantDestDir: "/reflinks/Aither", + wantAutoTMM: false, + }, { name: "new tracker category derives preset save path", candidateFiles: filesWithCommonRoot, categorySavePath: "", crossCategoryExistsInQbt: false,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/crossseed/link_mode_category_placement_test.go` around lines 26 - 53, Add a table-driven test case to internal/services/crossseed/link_mode_category_placement_test.go that covers the branch in resolveLinkModeCategoryPlacement where isTrackerCategoryMode==true, crossCategoryExistsInQbt==true and categorySavePath==""; set candidateFiles to an appropriate fixture (e.g. filesWithCommonRoot), leave categorySavePath empty, set crossCategoryExistsInQbt to true, and assert that the returned save path/destination falls back to the preset-derived path (same as the “new tracker category derives preset save path” expectation) and that AutoTMM is false (i.e. wantAutoTMM=false); update the test table (tests := []struct{...}) with a descriptive name like "existing mapped category with no save path falls back to preset and disables AutoTMM" so resolveLinkModeCategoryPlacement's behavior is verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/services/crossseed/hardlink_mode_test.go`:
- Around line 950-994: The test
TestProcessHardlinkMode_IgnoresCategorySavePathWhenNotTrackerCategoryMode can
accidentally call os.MkdirAll on an absolute host path ("/qbt/movies") if the
guard around isTrackerCategoryMode regresses; modify the test to create a
temporary blocker file/path (e.g., via os.CreateTemp or os.WriteFile) at a safe
temp location and point categorySavePath to that temp path so the category-path
branch deterministically fails without touching root, and ensure any temp files
are created with tight permissions (0o600) and cleaned up; verify the assertion
still checks that the error came from FindMatchingBaseDir and not the category
directory branch, and reference processHardlinkMode, FindMatchingBaseDir,
MkdirAll, categorySavePath and isTrackerCategoryMode when locating the code to
change.
In `@internal/services/crossseed/link_mode_category_placement_test.go`:
- Around line 26-53: Add a table-driven test case to
internal/services/crossseed/link_mode_category_placement_test.go that covers the
branch in resolveLinkModeCategoryPlacement where isTrackerCategoryMode==true,
crossCategoryExistsInQbt==true and categorySavePath==""; set candidateFiles to
an appropriate fixture (e.g. filesWithCommonRoot), leave categorySavePath empty,
set crossCategoryExistsInQbt to true, and assert that the returned save
path/destination falls back to the preset-derived path (same as the “new tracker
category derives preset save path” expectation) and that AutoTMM is false (i.e.
wantAutoTMM=false); update the test table (tests := []struct{...}) with a
descriptive name like "existing mapped category with no save path falls back to
preset and disables AutoTMM" so resolveLinkModeCategoryPlacement's behavior is
verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5ecdb4ac-8092-4593-802f-f881ce32f8da
📒 Files selected for processing (3)
internal/services/crossseed/hardlink_mode_test.gointernal/services/crossseed/link_mode_category_placement_test.gointernal/services/crossseed/service.go
✅ Files skipped from review due to trivial changes (1)
- internal/services/crossseed/service.go
CjE5
force-pushed
the
develop
branch
2 times, most recently
from
8f64f1b to
ffa75b0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/services/crossseed/tracker_category.go (1)
93-99:⚠️ Potential issue | 🟠 MajorRemove the loose substring fallback before it maps unrelated trackers.
The minimum-length guard still lets unrelated domains match by containment, e.g.
notbeyondhdcontainsbeyondhd. Since this match controls category/AutoTMM placement, keep only boundary/label-aware matches and explicit aliases for non-textual tracker domains.🐛 Proposed fix
- // Containment on fully separator-stripped strings: apply a minimum length - // guard on both sides to prevent short common tokens from matching broadly. - if len(normalizedIndexer) >= 6 && strings.Contains(domainNorm, normalizedIndexer) { - return true - } - if len(domainNorm) >= 6 && strings.Contains(normalizedIndexer, domainNorm) { - return true - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/crossseed/tracker_category.go` around lines 93 - 99, The loose substring containment fallback using normalizedIndexer and domainNorm is causing false positives (e.g., "notbeyondhd" matching "beyondhd"); remove the two containment checks (the if blocks checking len(normalizedIndexer) >= 6 && strings.Contains(domainNorm, normalizedIndexer) and len(domainNorm) >= 6 && strings.Contains(normalizedIndexer, domainNorm)) and rely only on the existing boundary/label-aware matching logic and explicit alias lookups for non-textual tracker domains; keep normalizedIndexer/domainNorm available for the stricter checks but do not perform raw substring Contains, and update/extend relevant tests to cover the removed fallback cases.
🧹 Nitpick comments (1)
internal/services/dirscan/service_tracker_category_test.go (1)
81-145: Fold these scenarios into a table-driven test.Both tests exercise
resolveDirscanTrackerCategorywith different setup/expectations; a table makes it easier to add the successful mapping and “no mapped category” cases without duplicating boilerplate. As per coding guidelines,**/*_test.go: Use table-driven test cases and reuse integration fixtures frominternal/qbittorrent/.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/dirscan/service_tracker_category_test.go` around lines 81 - 145, Combine the two tests into a single table-driven test that iterates over cases (e.g., "fallback on lookup error" and "fatal on get categories error"), each case specifying inputs like instanceID, trackerName, host, the fakeDirscanCategoryGetter.err value, expected fatal flag, expected category/savePath, and expected getter.calls; reuse the existing helpers setupTrackerCategoryDB and fakeDirscanCategoryGetter and call resolveDirscanTrackerCategory inside the loop, asserting outcomes per-case, and keep model setup (models.Instance, store) shared/parameterized to remove duplicated boilerplate while preserving the same assertions from TestResolveDirscanTrackerCategory_FallsBackOnLookupError and TestResolveDirscanTrackerCategory_FatalOnGetCategoriesError.
🤖 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/dirscan/service.go`:
- Around line 1808-1816: resolveDirscanTrackerCategory may receive a non-nil
interface holding a typed nil s.syncManager, causing a panic when
resolveDirscanTrackerCategory later calls dirscanCategoryGetter.GetCategories;
to fix, ensure you do not assign a typed-nil to the dirscanCategoryGetter
parameter by creating a local variable (e.g., var sm dirscanCategoryGetter) and
only set sm = s.syncManager when s.syncManager != nil, then pass sm to
resolveDirscanTrackerCategory instead of passing s.syncManager directly;
reference resolveDirscanTrackerCategory, s.syncManager, dirscanCategoryGetter,
and GetCategories.
---
Duplicate comments:
In `@internal/services/crossseed/tracker_category.go`:
- Around line 93-99: The loose substring containment fallback using
normalizedIndexer and domainNorm is causing false positives (e.g., "notbeyondhd"
matching "beyondhd"); remove the two containment checks (the if blocks checking
len(normalizedIndexer) >= 6 && strings.Contains(domainNorm, normalizedIndexer)
and len(domainNorm) >= 6 && strings.Contains(normalizedIndexer, domainNorm)) and
rely only on the existing boundary/label-aware matching logic and explicit alias
lookups for non-textual tracker domains; keep normalizedIndexer/domainNorm
available for the stricter checks but do not perform raw substring Contains, and
update/extend relevant tests to cover the removed fallback cases.
---
Nitpick comments:
In `@internal/services/dirscan/service_tracker_category_test.go`:
- Around line 81-145: Combine the two tests into a single table-driven test that
iterates over cases (e.g., "fallback on lookup error" and "fatal on get
categories error"), each case specifying inputs like instanceID, trackerName,
host, the fakeDirscanCategoryGetter.err value, expected fatal flag, expected
category/savePath, and expected getter.calls; reuse the existing helpers
setupTrackerCategoryDB and fakeDirscanCategoryGetter and call
resolveDirscanTrackerCategory inside the loop, asserting outcomes per-case, and
keep model setup (models.Instance, store) shared/parameterized to remove
duplicated boilerplate while preserving the same assertions from
TestResolveDirscanTrackerCategory_FallsBackOnLookupError and
TestResolveDirscanTrackerCategory_FatalOnGetCategoriesError.
🪄 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: 8d7fda82-6fbc-431e-9e6e-5aac762160b8
📒 Files selected for processing (34)
cmd/qui/main.gointernal/api/handlers/crossseed.gointernal/api/handlers/dirscan_webhook_test.gointernal/api/server.gointernal/api/server_test.gointernal/database/migrations/071_add_tracker_category_settings.sqlinternal/database/postgres_migrations/072_add_tracker_category_settings.sqlinternal/models/crossseed_indexer_categories.gointernal/models/crossseed_indexer_categories_test.gointernal/proxy/crossseed_indexer_categories_test.gointernal/proxy/handler.gointernal/proxy/handler_test.gointernal/services/crossseed/auto_tmm_test.gointernal/services/crossseed/category_lookup_failure_test.gointernal/services/crossseed/category_persist_path_test.gointernal/services/crossseed/crossseed_test.gointernal/services/crossseed/domain_aliases.gointernal/services/crossseed/domain_aliases_test.gointernal/services/crossseed/domain_matching_test.gointernal/services/crossseed/hardlink_mode_test.gointernal/services/crossseed/link_mode_category_placement_test.gointernal/services/crossseed/service.gointernal/services/crossseed/tracker_category.gointernal/services/crossseed/tracker_category_test.gointernal/services/dirscan/cancel_scan_test.gointernal/services/dirscan/inject.gointernal/services/dirscan/inject_test.gointernal/services/dirscan/service.gointernal/services/dirscan/service_tracker_category_test.gointernal/services/dirscan/webhook_queue_test.gointernal/web/swagger/openapi.yamlweb/src/lib/api.tsweb/src/pages/CrossSeedPage.tsxweb/src/types/index.ts
✅ Files skipped from review due to trivial changes (11)
- internal/api/server_test.go
- internal/services/crossseed/category_lookup_failure_test.go
- internal/services/crossseed/domain_matching_test.go
- internal/services/crossseed/tracker_category_test.go
- internal/database/postgres_migrations/072_add_tracker_category_settings.sql
- web/src/types/index.ts
- internal/proxy/crossseed_indexer_categories_test.go
- internal/models/crossseed_indexer_categories_test.go
- internal/services/crossseed/link_mode_category_placement_test.go
- internal/database/migrations/071_add_tracker_category_settings.sql
- internal/services/crossseed/category_persist_path_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- internal/services/dirscan/cancel_scan_test.go
- web/src/lib/api.ts
- internal/services/crossseed/hardlink_mode_test.go
- internal/models/crossseed_indexer_categories.go
- cmd/qui/main.go
- internal/services/crossseed/auto_tmm_test.go
- internal/services/crossseed/domain_aliases_test.go
- internal/web/swagger/openapi.yaml
- internal/services/crossseed/domain_aliases.go
- internal/services/dirscan/inject_test.go
- internal/services/dirscan/webhook_queue_test.go
- internal/services/crossseed/crossseed_test.go
- internal/services/crossseed/service.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/services/crossseed/service.go (1)
11220-11339:⚠️ Potential issue | 🟠 MajorDisable AutoTMM when category creation fails.
Both link-mode paths clear
crossCategoryafterensureCrossCategoryfails, butplacement.EnableAutoTMMcan remain true. That can add the torrent withautoTMM=trueand no category/savepath, so qBittorrent may place it outside the pre-created link tree.Proposed fix
@@ if crossCategory != "" { if err := s.ensureCrossCategory(ctx, candidate.InstanceID, crossCategory, effectiveCategorySavePath, false); err != nil { log.Warn().Err(err). Str("category", crossCategory). Str("savePath", effectiveCategorySavePath). Msg("[CROSSSEED] Hardlink mode: failed to ensure category exists, continuing without category") crossCategory = "" categoryCreationFailed = true } } @@ - if placement.EnableAutoTMM { + if placement.EnableAutoTMM && crossCategory != "" { options["autoTMM"] = "true" } else { options["autoTMM"] = "false" options["savepath"] = plan.RootDir } @@ if crossCategory != "" { if err := s.ensureCrossCategory(ctx, candidate.InstanceID, crossCategory, effectiveCategorySavePath, false); err != nil { log.Warn().Err(err). Str("category", crossCategory). Str("savePath", effectiveCategorySavePath). Msg("[CROSSSEED] Reflink mode: failed to ensure category exists, continuing without category") crossCategory = "" categoryCreationFailed = true } } @@ - if placement.EnableAutoTMM { + if placement.EnableAutoTMM && crossCategory != "" { options["autoTMM"] = "true" } else { options["autoTMM"] = "false" options["savepath"] = plan.RootDir }Also applies to: 11847-11955
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/crossseed/service.go` around lines 11220 - 11339, The bug: when ensureCrossCategory fails and crossCategory is cleared (see ensureCrossCategory, crossCategory, categoryCreationFailed), placement.EnableAutoTMM can still be true which would add the torrent with autoTMM=true and no category/savepath; fix by disabling AutoTMM when category creation fails—either set placement.EnableAutoTMM = false immediately after you set categoryCreationFailed = true (where ensureCrossCategory error is handled) or ensure later when building options you check categoryCreationFailed and force options["autoTMM"]="false" and options["savepath"]=plan.RootDir so qBittorrent uses the pre-created hardlink tree (refer to placement.EnableAutoTMM, options map, and plan.RootDir).
♻️ Duplicate comments (1)
internal/services/crossseed/service.go (1)
10111-10118:⚠️ Potential issue | 🟠 MajorDo not treat tracker-category lookup errors as no mapping.
This still falls back to global category rules when
ResolveTrackerCategoryreturns a store/context error. The fallback is safe for “no mapping found,” but not when the mapping state is unknown; surface the error so the inject can fail/retry instead. As per coding guidelines, “Prefer explicit error handling over silent failures in Go code”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/crossseed/service.go` around lines 10111 - 10118, The code currently treats any non-nil resolveErr from ResolveTrackerCategory(ctx, instance.ID, reqIndexerName, announceDomain, s.indexerCategoryStore) as a soft failure and falls through to global rules; instead, if resolveErr != nil return that error (or wrap and return it) from the enclosing function so the inject operation fails/retries rather than silently using a fallback. Locate the ResolveTrackerCategory call and replace the warning-only branch with explicit error handling that returns resolveErr (or a wrapped error including context such as instance.ID, reqIndexerName, announceDomain) rather than treating it as "not found"; keep the existing found branch behavior unchanged.
🧹 Nitpick comments (2)
internal/services/crossseed/tracker_category.go (1)
137-147: Minor: debug-level log for store error may hide issues.When
store.Listfails with a non-context error, the failure is logged atDebugonly and then propagated. Callers (e.g.,resolveDirscanTrackerCategory) do re-log atWarn, so this is fine in practice, but when this function is called from paths that treat the error as a soft fallback (e.g., return"", false, nildownstream), the DB failure could become invisible in default log setups. Considerlog.Warn()here, or leave as-is and ensure every caller re-logs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/crossseed/tracker_category.go` around lines 137 - 147, The error from store.List inside ResolveTrackerCategory is currently logged at Debug and can be missed; change the logging call to log.Warn() (keeping the same .Err(err).Int("instanceID", instanceID).Msg("[CROSSSEED] ResolveTrackerCategory: failed to list mappings") structure) so non-context DB failures are visible by default, ensuring callers like resolveDirscanTrackerCategory still handle/propagate the error as before.internal/services/dirscan/service_tracker_category_test.go (1)
81-145: Consider consolidating into a table-driven test.The two test functions share identical setup and call shape, differing only in (store source, getter error, expected fatal/calls). Per coding guidelines, prefer table-driven cases. Example consolidation:
♻️ Proposed refactor
func TestResolveDirscanTrackerCategory(t *testing.T) { t.Parallel() const instanceID = 42 tests := []struct { name string setupStore func(t *testing.T) *models.CrossSeedIndexerCategoryStore getter *fakeDirscanCategoryGetter wantFatal bool wantCalls int }{ { name: "falls back when store lookup errors", setupStore: func(t *testing.T) *models.CrossSeedIndexerCategoryStore { dbPath := filepath.Join(t.TempDir(), "indexer_categories.db") db, err := database.New(dbPath) require.NoError(t, err) store := models.NewCrossSeedIndexerCategoryStore(db) require.NoError(t, db.Close()) return store }, getter: &fakeDirscanCategoryGetter{err: errors.New("should not be called")}, wantFatal: false, wantCalls: 0, }, { name: "fatal when GetCategories errors", setupStore: func(t *testing.T) *models.CrossSeedIndexerCategoryStore { _, store := setupTrackerCategoryDB(t, instanceID, "Aither", "aither-movies") return store }, getter: &fakeDirscanCategoryGetter{err: errors.New("qbittorrent unavailable")}, wantFatal: true, wantCalls: 1, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { logger := zerolog.New(io.Discard) instance := &models.Instance{ID: instanceID, UseHardlinks: true, HardlinkDirPreset: "by-tracker"} store := tt.setupStore(t) category, savePath, fatal := resolveDirscanTrackerCategory( context.Background(), instanceID, "Aither", "aither.cc", instance, store, tt.getter, &logger, ) require.Equal(t, tt.wantFatal, fatal) require.Empty(t, category) require.Empty(t, savePath) require.Equal(t, tt.wantCalls, tt.getter.calls) }) } }As per coding guidelines: "Use table-driven test cases and reuse integration fixtures from
internal/qbittorrent/".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/dirscan/service_tracker_category_test.go` around lines 81 - 145, The two tests TestResolveDirscanTrackerCategory_FallsBackOnLookupError and TestResolveDirscanTrackerCategory_FatalOnGetCategoriesError are duplicated patterns; refactor into a single table-driven test that iterates cases and calls resolveDirscanTrackerCategory with different setupStore and getter values. Create a tests slice with fields (name, setupStore func(*testing.T) *models.CrossSeedIndexerCategoryStore, getter *fakeDirscanCategoryGetter, wantFatal bool, wantCalls int), use setupTrackerCategoryDB and the temp-db creation used in the original tests as the setupStore implementations, and assert fatal/category/savePath/getter.calls per case; keep using resolveDirscanTrackerCategory, fakeDirscanCategoryGetter and the original instance construction and logger.
🤖 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/crossseed/hardlink_mode_test.go`:
- Around line 978-983: Fix the gofmt violation by adjusting the spacing before
inline comments in the test case that contains the literals "movies",
"movies.cross", "/qbt/movies" and the trailing boolean comments (e.g., the
comment after "/qbt/movies" about categorySavePath and the comments for
isTrackerCategoryMode and crossCategoryExistsInQbit). Either collapse the double
spaces to a single space before each inline comment or run gofmt -w on the file
to apply canonical formatting so the line conforms to gofmt.
In `@internal/services/dirscan/inject.go`:
- Around line 601-613: The function mapTrackerCategorySavePathToHost (which uses
crossseed.FindMatchingBaseDir and applyInversePathMapping) can fail at runtime
when multiple HardlinkBaseDir entries span different filesystems but only a
single QbitPathPrefix is configured; add config-time validation when
saving/loading the HardlinkBaseDir + QbitPathPrefix pair to ensure the prefix
can inverse-map to at least one of the configured base dirs (or reject/save a
warning if base dirs live on multiple filesystems and the single QbitPathPrefix
cannot match all of them). Implement this by iterating the parsed
HardlinkBaseDir entries, calling applyInversePathMapping(qbitPathPrefix,
candidateBaseDir, qbitPathPrefix) (or equivalent test helper) to confirm at
least one successful mapping, and surface a clear validation error referencing
HardlinkBaseDir and QbitPathPrefix when no valid pairing exists; alternatively
add UI/API documentation notes next to the HardlinkBaseDir/QbitPathPrefix
configuration explaining the constraint.
---
Outside diff comments:
In `@internal/services/crossseed/service.go`:
- Around line 11220-11339: The bug: when ensureCrossCategory fails and
crossCategory is cleared (see ensureCrossCategory, crossCategory,
categoryCreationFailed), placement.EnableAutoTMM can still be true which would
add the torrent with autoTMM=true and no category/savepath; fix by disabling
AutoTMM when category creation fails—either set placement.EnableAutoTMM = false
immediately after you set categoryCreationFailed = true (where
ensureCrossCategory error is handled) or ensure later when building options you
check categoryCreationFailed and force options["autoTMM"]="false" and
options["savepath"]=plan.RootDir so qBittorrent uses the pre-created hardlink
tree (refer to placement.EnableAutoTMM, options map, and plan.RootDir).
---
Duplicate comments:
In `@internal/services/crossseed/service.go`:
- Around line 10111-10118: The code currently treats any non-nil resolveErr from
ResolveTrackerCategory(ctx, instance.ID, reqIndexerName, announceDomain,
s.indexerCategoryStore) as a soft failure and falls through to global rules;
instead, if resolveErr != nil return that error (or wrap and return it) from the
enclosing function so the inject operation fails/retries rather than silently
using a fallback. Locate the ResolveTrackerCategory call and replace the
warning-only branch with explicit error handling that returns resolveErr (or a
wrapped error including context such as instance.ID, reqIndexerName,
announceDomain) rather than treating it as "not found"; keep the existing found
branch behavior unchanged.
---
Nitpick comments:
In `@internal/services/crossseed/tracker_category.go`:
- Around line 137-147: The error from store.List inside ResolveTrackerCategory
is currently logged at Debug and can be missed; change the logging call to
log.Warn() (keeping the same .Err(err).Int("instanceID",
instanceID).Msg("[CROSSSEED] ResolveTrackerCategory: failed to list mappings")
structure) so non-context DB failures are visible by default, ensuring callers
like resolveDirscanTrackerCategory still handle/propagate the error as before.
In `@internal/services/dirscan/service_tracker_category_test.go`:
- Around line 81-145: The two tests
TestResolveDirscanTrackerCategory_FallsBackOnLookupError and
TestResolveDirscanTrackerCategory_FatalOnGetCategoriesError are duplicated
patterns; refactor into a single table-driven test that iterates cases and calls
resolveDirscanTrackerCategory with different setupStore and getter values.
Create a tests slice with fields (name, setupStore func(*testing.T)
*models.CrossSeedIndexerCategoryStore, getter *fakeDirscanCategoryGetter,
wantFatal bool, wantCalls int), use setupTrackerCategoryDB and the temp-db
creation used in the original tests as the setupStore implementations, and
assert fatal/category/savePath/getter.calls per case; keep using
resolveDirscanTrackerCategory, fakeDirscanCategoryGetter and the original
instance construction and logger.
🪄 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: 31deb310-f9a4-4042-9f59-8b4eb112ace0
📒 Files selected for processing (34)
cmd/qui/main.gointernal/api/handlers/crossseed.gointernal/api/handlers/dirscan_webhook_test.gointernal/api/server.gointernal/api/server_test.gointernal/database/migrations/071_add_tracker_category_settings.sqlinternal/database/postgres_migrations/072_add_tracker_category_settings.sqlinternal/models/crossseed_indexer_categories.gointernal/models/crossseed_indexer_categories_test.gointernal/proxy/crossseed_indexer_categories_test.gointernal/proxy/handler.gointernal/proxy/handler_test.gointernal/services/crossseed/auto_tmm_test.gointernal/services/crossseed/category_lookup_failure_test.gointernal/services/crossseed/category_persist_path_test.gointernal/services/crossseed/crossseed_test.gointernal/services/crossseed/domain_aliases.gointernal/services/crossseed/domain_aliases_test.gointernal/services/crossseed/domain_matching_test.gointernal/services/crossseed/hardlink_mode_test.gointernal/services/crossseed/link_mode_category_placement_test.gointernal/services/crossseed/service.gointernal/services/crossseed/tracker_category.gointernal/services/crossseed/tracker_category_test.gointernal/services/dirscan/cancel_scan_test.gointernal/services/dirscan/inject.gointernal/services/dirscan/inject_test.gointernal/services/dirscan/service.gointernal/services/dirscan/service_tracker_category_test.gointernal/services/dirscan/webhook_queue_test.gointernal/web/swagger/openapi.yamlweb/src/lib/api.tsweb/src/pages/CrossSeedPage.tsxweb/src/types/index.ts
✅ Files skipped from review due to trivial changes (10)
- internal/api/server_test.go
- web/src/types/index.ts
- internal/services/crossseed/category_persist_path_test.go
- internal/services/crossseed/domain_aliases.go
- internal/services/crossseed/category_lookup_failure_test.go
- internal/database/migrations/071_add_tracker_category_settings.sql
- internal/models/crossseed_indexer_categories_test.go
- internal/services/crossseed/tracker_category_test.go
- internal/services/crossseed/link_mode_category_placement_test.go
- internal/proxy/crossseed_indexer_categories_test.go
🚧 Files skipped from review as they are similar to previous changes (12)
- internal/services/dirscan/cancel_scan_test.go
- internal/api/handlers/dirscan_webhook_test.go
- cmd/qui/main.go
- internal/services/dirscan/webhook_queue_test.go
- internal/database/postgres_migrations/072_add_tracker_category_settings.sql
- internal/services/crossseed/domain_matching_test.go
- internal/services/crossseed/domain_aliases_test.go
- internal/services/dirscan/inject_test.go
- web/src/lib/api.ts
- internal/services/crossseed/auto_tmm_test.go
- internal/proxy/handler.go
- internal/services/dirscan/service.go
CjE5
force-pushed
the
develop
branch
3 times, most recently
from
9c8e97e to
c15c0da
Compare
CjE5
force-pushed
the
develop
branch
2 times, most recently
from
127445f to
0014490
Compare
CjE5
force-pushed
the
develop
branch
2 times, most recently
from
f51cff0 to
681669c
Compare
added 2 commits
May 23, 2026 11:27
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
2 participants
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.
This reverts commit 54fa047.
Trying this again. See: #1722
@s0up4200 any chance you could share the context from the channel in regards to the issues encountered? I think it's locked and I can't access it. I'd like to fix whatever issues seen and add required tests.
When a qBittorrent instance's directory organisation is set to "by-tracker", cross-seeds are assigned a category based on the (instance, indexer) mapping table rather than the global category mode. If no mapping is configured for the indexer, the existing global logic (custom → indexer-name → affix → reuse) is used as a fallback.
AutoTMM is enabled automatically when the mapped category has a configured save path in qBittorrent, ensuring files land in the right directory without moving the hardlinks.
The per-tracker mapping UI is shown inline under the instance's "Directory organisation" setting when "By Tracker" is selected.
A proxy-path endpoint (GET /api/v2/cross-seed/indexer-categories) is registered alongside the existing qBittorrent proxy routes so external cross-seed clients can resolve per-tracker categories using only the proxy key — no user API key required.
The same per-tracker category resolution and AutoTMM logic applies to directory-scan injections, not only the automation/webhook path.
Test coverage added for the indexer-category store (12 tests covering CRUD, upsert, whitespace trimming, case-insensitive lookup, instance isolation, and cascade delete), the proxy handler (4 tests), the autoTMM decision (3 new cases), and the announce-domain-to-indexer matching logic (20 cases).
#1631
Summary by CodeRabbit
New Features
Bug Fixes