feat(cross-seed): add "by-tracker" directory preset with per-indexer category mapping - #1722
Merged
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, per-indexer tracker-category persistence and resolution: DB migrations and store, API and proxy endpoints, cross-seed and dirscan service integration for "by-tracker" mode, injector/path-mapping changes, frontend client/UI, and tests. Wiring propagates a new CrossSeedIndexerCategoryStore through server, services, handlers, and proxy. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Web UI
participant API as API Server
participant Proxy as Proxy Handler
participant Service as CrossSeed Service
participant Dirscan as Dirscan Service
participant Store as IndexerCategoryStore
participant DB as Database
UI->>API: PUT /cross-seed/indexer-categories/{instanceId}
API->>Store: Set(ctx, instanceId, indexerId, category)
Store->>DB: INSERT ... ON CONFLICT UPDATE
DB-->>Store: OK
Store-->>API: OK
API-->>UI: 204
UI->>Proxy: GET /proxy/{api-key}/api/v2/cross-seed/indexer-categories
Proxy->>Store: List(ctx, instanceId)
Store->>DB: SELECT ... JOIN torznab_indexers,string_pool
DB-->>Store: mappings
Store-->>Proxy: mappings
Proxy-->>UI: 200 + JSON
UI->>API: Trigger cross-seed inject
API->>Service: processCrossSeedCandidate(ctx, req)
Service->>Store: ResolveTrackerCategory(ctx, instanceID, indexerName, announceDomain)
Store->>DB: SELECT ...
DB-->>Store: mapping or none
Store-->>Service: mapping or no-match
Service->>Dirscan: prepare InjectRequest (may include TrackerCategorySavePath)
Dirscan->>Dirscan: materializeLinkTree / buildAddOptions (uses TrackerCategorySavePath)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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: 8
🤖 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/crossseed.go`:
- Around line 1655-1739: Add instance existence validation before touching
indexerCategoryStore in GetInstanceIndexerCategories,
SetInstanceIndexerCategory, and DeleteInstanceIndexerCategory: after parsing
instanceID and before calling h.indexerCategoryStore.List/Set/Delete, call the
same instanceStore check used by other instance-scoped handlers (e.g.,
h.instanceStore.Get or Exists) and if the instance is missing respond with
RespondError(w, http.StatusNotFound, "instance not found"); keep the current
service-unavailable and store error handling otherwise so stale/deleted instance
IDs return 404 instead of empty results or 500s.
In `@internal/services/crossseed/service.go`:
- Around line 11223-11228: The current branch enables options["autoTMM"] when
isTrackerCategoryMode && categorySavePath != "" even if crossCategory was
cleared earlier; update the condition to also require the category assignment
(e.g., crossCategory != "" or a flag that indicates category was successfully
ensured) so autoTMM is only set to "true" when a valid category is present,
otherwise set options["autoTMM"]="false" and options["savepath"]=plan.RootDir as
the fallback. Ensure you reference and check the crossCategory (or the variable
that indicates category assignment) alongside isTrackerCategoryMode and
categorySavePath, and leave the fallback behavior for options["savepath"]
unchanged.
- Around line 4112-4116: The current code silently ignores errors from
s.GetAutomationSettings which can leave candidateSettings nil; change the block
around GetAutomationSettings to explicitly handle the error: call
s.GetAutomationSettings(ctx), and if err != nil log the error with context
(including that it occurred during candidate setup) and return or propagate the
error rather than swallowing it; otherwise assign candidateSettings =
autoSettings. Reference the GetAutomationSettings method, the candidateSettings
variable and the CrossSeedAutomationSettings type when making the change so the
error handling is applied at the candidate setup site.
In `@internal/services/crossseed/tracker_category.go`:
- Around line 73-80: The current symmetric substring checks using
strings.Contains on normalizedDomain and normalizedIndexer in
ResolveTrackerCategory are too permissive and cause short/common tokens to
mis-match; replace them with stricter boundary-aware checks: instead of
strings.Contains(normalizedDomain, normalizedIndexer) or vice versa, check for
exact equality or dot-delimited suffixes (e.g.,
strings.HasSuffix(normalizedDomain, "."+normalizedIndexer) or
strings.HasSuffix(normalizedIndexer, "."+normalizedDomain)) and/or split tokens
by non-alphanumeric characters and match full tokens only, ensuring you continue
to return true only for those tighter matches involving normalizedDomain and
normalizedIndexer.
- Around line 95-105: trackerDomainAliases lookup using normalizedDomain can
miss aliased hosts that include common prefixes (e.g., tracker., announce.,
www.); update the lookup logic in the block that references trackerDomainAliases
and normalizedDomain so you normalize candidate domains by stripping common
prefixes (at least "tracker.", "announce.", "www.") or generate prefix-stripped
variants before checking trackerDomainAliases, and also check each alias with
MatchAnnounceDomainToIndexer as before; ensure the code that iterates aliases
remains unchanged (MatchAnnounceDomainToIndexer, trackerDomainAliases,
normalizedDomain) but perform the prefix-normalization/variant generation prior
to the map lookup.
In `@internal/services/dirscan/inject.go`:
- Around line 448-455: TrackerCategorySavePath is a qBittorrent-facing path but
is being used directly for host filesystem ops; before computing
effectiveBaseDir and calling materializeLinkTree/os.MkdirAll, run the same
applyPathMapping used for savePath so TrackerCategorySavePath is translated to
the host-local path. Update the logic around effectiveBaseDir (which currently
picks req.TrackerCategorySavePath) to call
applyPathMapping(req.TrackerCategorySavePath, instance.QbitPathPrefix,
instance.HostPathPrefix or the existing mapping args) and then fall back to
instance.HardlinkBaseDir if the mapped result is empty; ensure
materializeLinkTree receives the mapped path.
In `@internal/services/dirscan/service.go`:
- Around line 1785-1805: The branch that applies persisted tracker mappings uses
crossseed.ResolveTrackerCategory and s.indexerCategoryStore to override the
local variable category and then calls s.syncManager.GetCategories to fetch
qBittorrent save paths, but it is missing a guard for the active "by-tracker"
preset and silently proceeds when GetCategories fails; update the logic to first
check the instance's active preset (the same preset variable used elsewhere in
this function) and only attempt ResolveTrackerCategory when the preset is
"by-tracker", and if s.syncManager.GetCategories(ctx, dir.TargetInstanceID)
returns an error or does not contain the tracker category, do not mutate
category or set trackerCategorySavePath — instead fail closed or explicitly fall
back to the existing behavior before assigning category so you never tag a
torrent with a tracker category unless you have a valid save path.
In `@web/src/pages/CrossSeedPage.tsx`:
- Around line 530-552: The onClick handler clears the user's input immediately
after calling setMutation.mutate which loses the input if the mutation fails;
instead, remove setAddingRow(null) from the onClick and clear the state only
after a successful mutation by placing setAddingRow(null) in the setMutation's
onSuccess callback (or pass an onSuccess option to setMutation.mutate) so
addingRow is only cleared when the mutation succeeds; update references to
addingRow, setMutation, and setAddingRow accordingly.
🪄 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: 31c1c629-63f2-4466-8c27-4e80700f0bec
📒 Files selected for processing (25)
cmd/qui/main.gointernal/api/handlers/crossseed.gointernal/api/handlers/dirscan_webhook_test.gointernal/api/server.gointernal/api/server_test.gointernal/database/migrations/069_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/crossseed_test.gointernal/services/crossseed/domain_aliases.gointernal/services/crossseed/domain_aliases_test.gointernal/services/crossseed/hardlink_mode_test.gointernal/services/crossseed/service.gointernal/services/crossseed/tracker_category.gointernal/services/crossseed/tracker_category_test.gointernal/services/dirscan/inject.gointernal/services/dirscan/service.gointernal/services/dirscan/webhook_queue_test.goweb/src/lib/api.tsweb/src/pages/CrossSeedPage.tsxweb/src/types/index.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
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 (1)
11045-11121:⚠️ Potential issue | 🟠 MajorReuse the resolved
categorySavePathonce it has already replacedHardlinkBaseDir.After Line 11050,
selectedBaseDiris the mapped category directory itself. The laterbuildCategorySavePath(ctx, instance, selectedBaseDir, ...)call then re-applies the preset and can create/validatecrossCategoryagainst<mapped-path>/<tracker|instance>whiledestDirand the add options still use the original mapped path. That leaves qBittorrent's category save path pointing somewhere other than the files you just hardlinked.💡 Possible fix
categoryCreationFailed := false if crossCategory != "" { - categorySavePath := s.buildCategorySavePath(ctx, instance, selectedBaseDir, incomingTrackerDomain, candidate, req) - if err := s.ensureCrossCategory(ctx, candidate.InstanceID, crossCategory, categorySavePath); err != nil { + desiredCategorySavePath := categorySavePath + if desiredCategorySavePath == "" { + desiredCategorySavePath = s.buildCategorySavePath(ctx, instance, selectedBaseDir, incomingTrackerDomain, candidate, req) + } + if err := s.ensureCrossCategory(ctx, candidate.InstanceID, crossCategory, desiredCategorySavePath); err != nil { log.Warn().Err(err). Str("category", crossCategory). - Str("savePath", categorySavePath). + Str("savePath", desiredCategorySavePath). Msg("[CROSSSEED] Hardlink mode: failed to ensure category exists, continuing without category") crossCategory = "" categoryCreationFailed = true } }🤖 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 11045 - 11121, The code re-computes a category save path with buildCategorySavePath even when a concrete categorySavePath was provided and already assigned to selectedBaseDir, causing qBittorrent to be configured with a different path than the hardlinked files; fix by reusing the resolved selectedBaseDir as the category path when the original categorySavePath was non-empty (i.e. set categorySavePath = selectedBaseDir or skip calling buildCategorySavePath in that branch) before calling ensureCrossCategory (referencing selectedBaseDir, buildCategorySavePath, ensureCrossCategory, crossCategory, and categorySavePath).internal/proxy/handler.go (1)
412-415:⚠️ Potential issue | 🟠 MajorScope this endpoint outside proxy-context middleware.
/api/v2/cross-seed/indexer-categoriesis currently behindprepareProxyContextMiddleware, so it can fail with proxy-context 502s even though it only needs API-key-derivedinstanceID+ DB lookup. This makes category resolution unavailable for reasons unrelated to mappings.🧩 Proposed routing split
proxyRouter.Route(proxyRoute, func(pr chi.Router) { - // Apply proxy context middleware (adds instance info to context) - pr.Use(h.prepareProxyContextMiddleware) + // qui-only endpoint: requires validated proxy key context, but not qB proxy context + pr.Get("/api/v2/cross-seed/indexer-categories", h.handleCrossSeedIndexerCategories) + + // Endpoints that require full proxy context + pr.Group(func(pr chi.Router) { + pr.Use(h.prepareProxyContextMiddleware) - pr.Post("/api/v2/torrents/reannounce", h.handleReannounce) + pr.Post("/api/v2/torrents/reannounce", h.handleReannounce) ... - // cross-seed extensions (qui-specific, not forwarded to qBittorrent) - pr.Get("/api/v2/cross-seed/indexer-categories", h.handleCrossSeedIndexerCategories) - - // Handle the base proxy path and any nested paths requested through the proxy - pr.HandleFunc("/", h.ServeHTTP) - pr.HandleFunc("/*", h.ServeHTTP) + // Handle the base proxy path and any nested paths requested through the proxy + pr.HandleFunc("/", h.ServeHTTP) + pr.HandleFunc("/*", h.ServeHTTP) + }) })Also applies to: 437-438
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/proxy/handler.go` around lines 412 - 415, The route for /api/v2/cross-seed/indexer-categories is currently registered inside the proxy Router block that applies prepareProxyContextMiddleware (see proxyRouter.Route(proxyRoute, ...) and h.prepareProxyContextMiddleware) causing unnecessary 502s; move the registration for that specific endpoint outside of the block that calls pr.Use(h.prepareProxyContextMiddleware) (or register it on the parent router before the middleware is applied) so it uses only API-key-derived instanceID + DB lookup; update both occurrences referenced around the same area (also applies to the registration at the lines noted ~437-438) to ensure this endpoint is not wrapped by prepareProxyContextMiddleware.
🧹 Nitpick comments (3)
internal/services/dirscan/inject_test.go (1)
604-609: Test case name is misleading.The test case is named "round-trips with applyPathMapping" but doesn't actually verify round-trip behavior. It only tests
applyInversePathMappingin isolation. A true round-trip test would verify that applying both functions in sequence returns the original value.🔧 Suggested fix: rename or expand the test case
Option 1 - Rename to reflect actual behavior:
{ - name: "round-trips with applyPathMapping", + name: "basic prefix substitution", qbitPath: "/downloads/Movie.Name", searcheePath: "/data/torrents", qbitPrefix: "/downloads", want: "/data/torrents/Movie.Name", },Option 2 - Add an actual round-trip verification test:
func TestPathMappingRoundTrip(t *testing.T) { searcheePath := "/data/torrents" qbitPrefix := "/downloads" original := "/data/torrents/Movie.Name" // Forward: host path → qbit path mapped := applyPathMapping(original, searcheePath, qbitPrefix) // Inverse: qbit path → host path restored := applyInversePathMapping(mapped, searcheePath, qbitPrefix) if restored != original { t.Errorf("round-trip failed: %q → %q → %q", original, mapped, restored) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/dirscan/inject_test.go` around lines 604 - 609, The test case named "round-trips with applyPathMapping" is misleading because it only calls applyInversePathMapping; either rename the case to reflect it tests only the inverse mapping (e.g., "applyInversePathMapping maps qbit→host") or add a true round-trip assertion: call applyPathMapping(original, searcheePath, qbitPrefix) then applyInversePathMapping(mapped, searcheePath, qbitPrefix) and assert the final value equals the original; update the test case name or add a new TestPathMappingRoundTrip that references applyPathMapping and applyInversePathMapping accordingly.internal/api/handlers/crossseed.go (1)
1668-1679: Consider extracting shared instance-validation logic.The same
instanceStore.Get+ not-found/internal-error handling is duplicated in three handlers. A small helper would reduce drift risk.♻️ Suggested refactor
+func (h *CrossSeedHandler) validateInstanceExists(w http.ResponseWriter, r *http.Request, instanceID int, scope string) bool { + if h.instanceStore == nil { + return true + } + _, err := h.instanceStore.Get(r.Context(), instanceID) + if err == nil { + return true + } + if errors.Is(err, models.ErrInstanceNotFound) { + RespondError(w, http.StatusNotFound, "Instance not found") + return false + } + log.Error().Err(err).Int("instanceID", instanceID).Str("scope", scope).Msg("Failed to validate instance") + RespondError(w, http.StatusInternalServerError, "Failed to validate instance") + return false +}Also applies to: 1704-1715, 1758-1769
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handlers/crossseed.go` around lines 1668 - 1679, Extract the repeated instance existence check into a single helper on the handler (e.g., a method like validateInstance or ensureInstanceExists) that accepts ctx, instanceID and returns either nil/on-success or a typed error (or a bool plus an error) so callers can respond appropriately; inside the helper call h.instanceStore.Get(ctx, instanceID), map models.ErrInstanceNotFound to a not-found sentinel and return other errors after logging with log.Error().Err(err).Int("instanceID", instanceID).Msg(...), and update the three handler sites (the block using h.instanceStore.Get, the blocks around instanceID at the locations referenced) to call this helper and use RespondError only in the handler based on the helper's result.internal/proxy/handler.go (1)
2130-2144: Use JSON error responses for this API endpoint.This handler returns plain-text
http.Error, while neighboring proxy API handlers already havewriteJSONError. Aligning error format avoids client-side special-casing.📦 Suggested response-shape alignment
if instanceID == 0 { - http.Error(w, "missing instance context", http.StatusInternalServerError) + writeJSONError(w, http.StatusInternalServerError, "missing instance context") return } if h.indexerCategoryStore == nil { - http.Error(w, "indexer category store not available", http.StatusServiceUnavailable) + writeJSONError(w, http.StatusServiceUnavailable, "indexer category store not available") return } mappings, err := h.indexerCategoryStore.List(ctx, instanceID) if err != nil { log.Error().Err(err).Int("instanceId", instanceID).Msg("Failed to list indexer categories for proxy request") - http.Error(w, "failed to load indexer categories", http.StatusInternalServerError) + writeJSONError(w, http.StatusInternalServerError, "failed to load indexer categories") return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/proxy/handler.go` around lines 2130 - 2144, Replace all plain-text http.Error responses in the handler branch that checks instanceID, h.indexerCategoryStore, and after h.indexerCategoryStore.List(ctx, instanceID) with the API's JSON error helper (writeJSONError) so the endpoint returns consistent JSON error shapes; specifically, for the "missing instance context", "indexer category store not available", and the List error case, call writeJSONError with the appropriate HTTP status and error message and include the underlying error (err) or instanceID in the log context when handling the List failure (matching neighboring proxy handlers' usage).
🤖 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/server_test.go`:
- Around line 71-73: The three routes currently added to undocumentedRoutes (the
GET and PUT for "/api/cross-seed/indexer-categories/{instanceId}" and the DELETE
for "/api/cross-seed/indexer-categories/{instanceId}/{indexerID}" in
internal/api/server_test.go) should be removed from the allowlist and instead
documented in the OpenAPI spec under internal/web/swagger: add the corresponding
path entries, operations, security requirements and schemas to the swagger
YAML/JSON so the endpoints are part of the authenticated /api surface, then run
and fix failures from make test-openapi to ensure the OpenAPI content matches
the code.
In `@internal/services/crossseed/service.go`:
- Around line 10072-10081: When calling s.indexerCategoryStore.GetByIndexerName
in the fallback block, explicitly distinguish an operational error from a "no
mapping" result: if err != nil, log it with context (include instance.ID and
req.IndexerName and the error via log.Error().Err(err)) so lookup failures are
visible, and only when err == nil and cat != "" use the existing log.Info and
return cat; do not let an internal store error silently fall through to
global-category logic. Use the existing GetByIndexerName call/site and the
log.Info block as the insertion point for the new log.Error handling.
- Around line 4646-4648: The current tracker-mode check (isTrackerCategoryMode
:= instance != nil && instance.HardlinkDirPreset == "by-tracker") enables
tracker-category AutoTMM on the reuse path; change it to only enable
tracker-mode when the tracker category path has actually been populated/used by
the torrent by adding a requirement that actualCategorySavePath equals
props.SavePath (i.e., isTrackerCategoryMode := instance != nil &&
instance.HardlinkDirPreset == "by-tracker" && actualCategorySavePath ==
props.SavePath) so that shouldEnableAutoTMM(...) won't flip AutoTMM to
tracker-mode when the reuse path still points at the original savePath.
In `@internal/services/crossseed/tracker_category_test.go`:
- Line 37: The test case row where name is "domain with .org TLD" has a
mismatched TLD: announceDomain is "broadcasthe.net" (.net), so update the test
case's name string to accurately reflect the .net TLD (e.g., "domain with .net
TLD") so the test description matches announceDomain; locate the test table
entry using the fields name and announceDomain in tracker_category_test.go and
change only the name text.
In `@internal/services/dirscan/inject.go`:
- Around line 391-406: applyInversePathMapping currently does a raw prefix cut
which allows false matches like "/downloads-tv" matching "/downloads" and can
drop separators; change it to normalize qbitPath and qbitPrefix with
filepath.Clean, then only accept a mapping when qbitPath == qbitPrefix or
qbitPath has the exact directory-boundary prefix qbitPrefix +
string(os.PathSeparator); on successful match return filepath.Join(searcheePath,
suffixWithoutLeadingSep) (or filepath.Clean of searcheePath+suffix) but on
non-exact matches return an empty string (or an error sentinel) so callers (the
MkdirAll/filesystem/link-tree code paths referenced around lines 464-475 and
495-503) treat it as an unmappable path and fail closed rather than creating
links in the wrong host directory.
---
Outside diff comments:
In `@internal/proxy/handler.go`:
- Around line 412-415: The route for /api/v2/cross-seed/indexer-categories is
currently registered inside the proxy Router block that applies
prepareProxyContextMiddleware (see proxyRouter.Route(proxyRoute, ...) and
h.prepareProxyContextMiddleware) causing unnecessary 502s; move the registration
for that specific endpoint outside of the block that calls
pr.Use(h.prepareProxyContextMiddleware) (or register it on the parent router
before the middleware is applied) so it uses only API-key-derived instanceID +
DB lookup; update both occurrences referenced around the same area (also applies
to the registration at the lines noted ~437-438) to ensure this endpoint is not
wrapped by prepareProxyContextMiddleware.
In `@internal/services/crossseed/service.go`:
- Around line 11045-11121: The code re-computes a category save path with
buildCategorySavePath even when a concrete categorySavePath was provided and
already assigned to selectedBaseDir, causing qBittorrent to be configured with a
different path than the hardlinked files; fix by reusing the resolved
selectedBaseDir as the category path when the original categorySavePath was
non-empty (i.e. set categorySavePath = selectedBaseDir or skip calling
buildCategorySavePath in that branch) before calling ensureCrossCategory
(referencing selectedBaseDir, buildCategorySavePath, ensureCrossCategory,
crossCategory, and categorySavePath).
---
Nitpick comments:
In `@internal/api/handlers/crossseed.go`:
- Around line 1668-1679: Extract the repeated instance existence check into a
single helper on the handler (e.g., a method like validateInstance or
ensureInstanceExists) that accepts ctx, instanceID and returns either
nil/on-success or a typed error (or a bool plus an error) so callers can respond
appropriately; inside the helper call h.instanceStore.Get(ctx, instanceID), map
models.ErrInstanceNotFound to a not-found sentinel and return other errors after
logging with log.Error().Err(err).Int("instanceID", instanceID).Msg(...), and
update the three handler sites (the block using h.instanceStore.Get, the blocks
around instanceID at the locations referenced) to call this helper and use
RespondError only in the handler based on the helper's result.
In `@internal/proxy/handler.go`:
- Around line 2130-2144: Replace all plain-text http.Error responses in the
handler branch that checks instanceID, h.indexerCategoryStore, and after
h.indexerCategoryStore.List(ctx, instanceID) with the API's JSON error helper
(writeJSONError) so the endpoint returns consistent JSON error shapes;
specifically, for the "missing instance context", "indexer category store not
available", and the List error case, call writeJSONError with the appropriate
HTTP status and error message and include the underlying error (err) or
instanceID in the log context when handling the List failure (matching
neighboring proxy handlers' usage).
In `@internal/services/dirscan/inject_test.go`:
- Around line 604-609: The test case named "round-trips with applyPathMapping"
is misleading because it only calls applyInversePathMapping; either rename the
case to reflect it tests only the inverse mapping (e.g.,
"applyInversePathMapping maps qbit→host") or add a true round-trip assertion:
call applyPathMapping(original, searcheePath, qbitPrefix) then
applyInversePathMapping(mapped, searcheePath, qbitPrefix) and assert the final
value equals the original; update the test case name or add a new
TestPathMappingRoundTrip that references applyPathMapping and
applyInversePathMapping accordingly.
🪄 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: ce6ca8f2-66f0-4266-a8a6-c5e6c4bd9612
📒 Files selected for processing (26)
cmd/qui/main.gointernal/api/handlers/crossseed.gointernal/api/handlers/dirscan_webhook_test.gointernal/api/server.gointernal/api/server_test.gointernal/database/migrations/069_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/crossseed_test.gointernal/services/crossseed/domain_aliases.gointernal/services/crossseed/domain_aliases_test.gointernal/services/crossseed/hardlink_mode_test.gointernal/services/crossseed/service.gointernal/services/crossseed/tracker_category.gointernal/services/crossseed/tracker_category_test.gointernal/services/dirscan/inject.gointernal/services/dirscan/inject_test.gointernal/services/dirscan/service.gointernal/services/dirscan/webhook_queue_test.goweb/src/lib/api.tsweb/src/pages/CrossSeedPage.tsxweb/src/types/index.ts
✅ Files skipped from review due to trivial changes (7)
- internal/api/handlers/dirscan_webhook_test.go
- cmd/qui/main.go
- web/src/types/index.ts
- internal/database/migrations/069_add_tracker_category_settings.sql
- internal/proxy/crossseed_indexer_categories_test.go
- internal/services/crossseed/domain_aliases.go
- web/src/pages/CrossSeedPage.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/services/dirscan/webhook_queue_test.go
- internal/services/crossseed/domain_aliases_test.go
- internal/proxy/handler_test.go
- internal/services/crossseed/auto_tmm_test.go
- internal/models/crossseed_indexer_categories_test.go
- internal/services/crossseed/tracker_category.go
- internal/models/crossseed_indexer_categories.go
- internal/services/crossseed/hardlink_mode_test.go
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 (1)
internal/services/crossseed/service.go (1)
4447-4465:⚠️ Potential issue | 🟠 MajorDon't seed tracker categories with the matched torrent's save path.
In
"by-tracker"mode, Lines 4448-4450 fall back toprops.SavePathwhen the mapped category has no explicit qB save path, and Lines 4456-4465 then persist that value viaensureCrossCategory. That turns an unconfigured tracker category into a configured one rooted at the source library path, so later adds can wrongly treat it as tracker-managed and flip AutoTMM against the wrong directory.💡 Possible fix
- if categorySavePath == "" { - categorySavePath = props.SavePath - } + if categorySavePath == "" && !(instance != nil && instance.HardlinkDirPreset == "by-tracker") { + categorySavePath = props.SavePath + }If creating a missing tracker category with an empty path is not desired here, skip
ensureCrossCategoryentirely for that branch instead of persistingprops.SavePath.🤖 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 4447 - 4465, The code currently falls back to props.SavePath when categorySavePath is empty and then calls ensureCrossCategory, which persists that fallback path; change the logic to detect when categorySavePath came from the fallback (e.g., set a boolean like savePathWasFallback before assigning categorySavePath = props.SavePath) and skip calling ensureCrossCategory if savePathWasFallback (or simply leave crossCategory empty) so you don't create/configure a tracker category with the matched torrent's save path; update any related flags (e.g., categoryCreationFailed) accordingly and keep existing checks for useReflinkMode/useHardlinkMode around the ensureCrossCategory call.
🧹 Nitpick comments (2)
web/src/pages/CrossSeedPage.tsx (2)
591-599: Indexer query duplicated but safely deduplicated.This query uses the same
queryKeyas the one inCrossSeedPage(line 973), so React Query shares the cache. Not an issue, but ifHardlinkModeSettingswere extracted to its own file, consider acceptingenabledIndexersas a prop to avoid the implicit coupling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/CrossSeedPage.tsx` around lines 591 - 599, The local useQuery that derives enabledIndexers duplicates the cache keyed by ["torznab","indexers"], creating implicit coupling; refactor HardlinkModeSettings to accept enabledIndexers as a prop (remove the internal useQuery and useMemo), then update CrossSeedPage (and any other callers) to pass the computed enabledIndexers into HardlinkModeSettings so the component no longer performs its own query and avoids implicit shared-cache coupling.
480-487: Delete button lacks a disabled state during pending mutation.Rapid clicks can queue multiple delete requests for the same mapping. Consider disabling while
deleteMutation.isPendingto prevent duplicate API calls.♻️ Proposed fix
<Button variant="ghost" size="icon" className="h-7 w-7 shrink-0" onClick={() => deleteMutation.mutate(mapping.indexerId)} + disabled={deleteMutation.isPending} > <X className="h-3.5 w-3.5" /> </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/CrossSeedPage.tsx` around lines 480 - 487, The delete button currently allows rapid repeated clicks because it doesn't disable while a delete is in progress; update the Button that calls deleteMutation.mutate(mapping.indexerId) to set disabled={deleteMutation.isPending} (or use deleteMutation.isLoading if your mutation API uses that) and optionally add aria-disabled for accessibility, so the Button (and the X icon) cannot be clicked again until the mutation completes.
🤖 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/service.go`:
- Around line 11056-11073: The selectedBaseDir is already the final category
directory when categorySavePath is set, but later callers re-run
buildCategorySavePath(...) causing the tracker/instance segment to be appended
twice; update the code paths that use selectedBaseDir (including the call site
around buildCategorySavePath at lines referenced and the logic handling
createdCategories / ensureCrossCategory) so that when selectedBaseDir was taken
from categorySavePath you skip calling buildCategorySavePath and treat the path
as final (do not attempt repair or treat ensureCrossCategory warn-only cases as
failures); specifically, detect when categorySavePath != "" (or when
selectedBaseDir == categorySavePath) and avoid re-appending tracker/instance
segments and avoid marking category creation as failed in
createdCategories/ensureCrossCategory handling.
In `@internal/services/crossseed/tracker_category.go`:
- Around line 43-50: The stripTrackerTLD function's TLD list is missing ".co",
so stripTrackerTLD(domain) will leave "beyondhd.co" unchanged; update the slice
in stripTrackerTLD to include ".co" among the suffixes (the list in the function
that currently contains ".cc", ".org", ".net", ".com", ".to", ".me", ".tv",
".xyz") so that strings.CutSuffix will strip ".co" and allow subsequent matching
steps to succeed.
In `@internal/web/swagger/openapi.yaml`:
- Around line 3945-4036: Add the missing proxy API paths for cross-seed indexer
categories so external clients have a complete OpenAPI contract: duplicate the
three documented routes under the proxy prefix
(/proxy/{api-key}/api/v2/cross-seed/indexer-categories and
/proxy/{api-key}/api/v2/cross-seed/indexer-categories/{indexerID}) including the
same operations (GET, PUT, DELETE), request/response schemas
(CrossSeedIndexerCategory, CrossSeedIndexerCategoryRequest), and response codes;
declare the {api-key} path parameter (type: string, required) and ensure any
security requirements for the proxy route are represented; run and fix any
issues from make test-openapi to validate the spec.
- Around line 3952-3957: The path parameter declarations for instanceId in
openapi.yaml lack a positivity constraint—update each affected parameter block
(the three occurrences around the given diff and also the ones at the ranges
noted) to require a positive integer by adding "minimum: 1" to the schema or
replace each inline parameter with a reference to the shared parameter
definition "#/components/parameters/instanceID" that already enforces minimum:
1; ensure you modify the parameter named "instanceId" (the path param blocks) so
all path params consistently enforce instanceId >= 1.
---
Outside diff comments:
In `@internal/services/crossseed/service.go`:
- Around line 4447-4465: The code currently falls back to props.SavePath when
categorySavePath is empty and then calls ensureCrossCategory, which persists
that fallback path; change the logic to detect when categorySavePath came from
the fallback (e.g., set a boolean like savePathWasFallback before assigning
categorySavePath = props.SavePath) and skip calling ensureCrossCategory if
savePathWasFallback (or simply leave crossCategory empty) so you don't
create/configure a tracker category with the matched torrent's save path; update
any related flags (e.g., categoryCreationFailed) accordingly and keep existing
checks for useReflinkMode/useHardlinkMode around the ensureCrossCategory call.
---
Nitpick comments:
In `@web/src/pages/CrossSeedPage.tsx`:
- Around line 591-599: The local useQuery that derives enabledIndexers
duplicates the cache keyed by ["torznab","indexers"], creating implicit
coupling; refactor HardlinkModeSettings to accept enabledIndexers as a prop
(remove the internal useQuery and useMemo), then update CrossSeedPage (and any
other callers) to pass the computed enabledIndexers into HardlinkModeSettings so
the component no longer performs its own query and avoids implicit shared-cache
coupling.
- Around line 480-487: The delete button currently allows rapid repeated clicks
because it doesn't disable while a delete is in progress; update the Button that
calls deleteMutation.mutate(mapping.indexerId) to set
disabled={deleteMutation.isPending} (or use deleteMutation.isLoading if your
mutation API uses that) and optionally add aria-disabled for accessibility, so
the Button (and the X icon) cannot be clicked again until the mutation
completes.
🪄 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: bd7998a1-fb80-481b-8807-a38708b394ed
📒 Files selected for processing (28)
cmd/qui/main.gointernal/api/handlers/crossseed.gointernal/api/handlers/dirscan_webhook_test.gointernal/api/server.gointernal/api/server_test.gointernal/database/migrations/069_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/crossseed_test.gointernal/services/crossseed/domain_aliases.gointernal/services/crossseed/domain_aliases_test.gointernal/services/crossseed/hardlink_mode_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/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 (7)
- internal/services/dirscan/webhook_queue_test.go
- internal/api/handlers/dirscan_webhook_test.go
- cmd/qui/main.go
- internal/services/crossseed/crossseed_test.go
- internal/services/crossseed/domain_aliases.go
- internal/database/migrations/069_add_tracker_category_settings.sql
- internal/proxy/crossseed_indexer_categories_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/proxy/handler_test.go
- internal/api/server.go
- internal/services/crossseed/domain_aliases_test.go
- internal/services/crossseed/hardlink_mode_test.go
- internal/services/crossseed/tracker_category_test.go
- internal/services/dirscan/inject_test.go
- internal/services/crossseed/auto_tmm_test.go
- internal/models/crossseed_indexer_categories.go
- internal/proxy/handler.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/services/dirscan/service.go (1)
113-125:⚠️ Potential issue | 🔴 CriticalUpdate the
NewServicewiring before this lands.
NewServicenow requiresindexerCategoryStore, butcmd/qui/main.go:646-651still callsdirscan.NewService(...)with the old 9-argument signature. As shown, this change will not compile until that production call site is updated too.🤖 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 113 - 125, The production call site that constructs the directory scanner service still uses the old 9-argument signature for NewService; update the call to match the new signature by supplying the new indexerCategoryStore argument (type *models.CrossSeedIndexerCategoryStore) — pass the actual indexerCategoryStore variable from your startup wiring or nil if it is intentionally optional; ensure the call to dirscan.NewService includes this additional parameter so the constructor signature and call sites match.
♻️ Duplicate comments (2)
internal/services/crossseed/tracker_category.go (1)
73-86:⚠️ Potential issue | 🟠 MajorRaw substring matching still misroutes short indexer names.
Line 75 and Line 85 still accept arbitrary embedded tokens, so inputs like
MatchAnnounceDomainToIndexer("flacsfor.me", "FL")return true and can steal the category before alias matching runs. Compare normalized host labels or boundary-delimited tokens instead of using rawstrings.Contains.Suggested fix
- // 2. Partial: lower-trimmed domain contains normalized indexer name. - // Handles "tracker.beyondhd.co" ⊇ "beyondhd". - if strings.Contains(normalizedDomain, normalizedIndexer) { - return true - } + // 2. Match normalized host labels instead of arbitrary substrings. + for _, label := range strings.Split(normalizedDomain, ".") { + if normalizeDomainNameValue(label) == normalizedIndexer { + return true + } + } @@ - if strings.Contains(domainNorm, normalizedIndexer) { - return true - } // Minimum length guard prevents short tokens (e.g. "hd", "tv") from // spuriously matching unrelated indexers via a suffix fragment. if len(domainNorm) >= 6 && strings.HasPrefix(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 73 - 86, The current raw substring checks in MatchAnnounceDomainToIndexer (the strings.Contains uses around normalizedDomain and domainNorm) misroute short indexer names; change them to token/boundary-aware comparisons: split normalizedDomain and domainNorm into labels/tokens using separator normalization (call normalizeDomainNameValue and split on non-alphanumerics or use strings.FieldsFunc) and then check for exact token equality with normalizedIndexer (or compare host labels returned by stripTrackerTLD) instead of using strings.Contains; update the two places that currently call strings.Contains(normalizedDomain, normalizedIndexer) and strings.Contains(domainNorm, normalizedIndexer) to iterate tokens and return true only when a token == normalizedIndexer.internal/services/dirscan/service.go (1)
1796-1809:⚠️ Potential issue | 🟠 MajorOnly switch to the tracker category after qBittorrent confirms it exists.
category = trackerCatstill runs whenqbitCatsdoes not contain that category, which leavestrackerCategorySavePathempty. That puts the inject path back on the generic fallback while still tagging the torrent with the tracker-specific category.Suggested guard
- } else { - category = trackerCat - if cat, ok := qbitCats[trackerCat]; ok { - trackerCategorySavePath = cat.SavePath - } - l.Info(). - Str("announceDomain", announceDomain). - Str("trackerCategory", trackerCat). - Str("categorySavePath", trackerCategorySavePath). - Msg("dirscan: resolved by-tracker category") + } else if cat, ok := qbitCats[trackerCat]; ok { + category = trackerCat + trackerCategorySavePath = cat.SavePath + l.Info(). + Str("announceDomain", announceDomain). + Str("trackerCategory", trackerCat). + Str("categorySavePath", trackerCategorySavePath). + Msg("dirscan: resolved by-tracker category") + } else { + l.Warn(). + Str("trackerCategory", trackerCat). + Msg("dirscan: tracker category missing in qBittorrent, skipping by-tracker override") }🤖 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 1796 - 1809, The code sets category = trackerCat even when qbitCats doesn't contain trackerCat, leaving trackerCategorySavePath empty; change the logic in the ResolveTrackerCategory handling so you only assign category = trackerCat (and set trackerCategorySavePath = cat.SavePath) when qbitCats contains trackerCat (i.e. the lookup qbitCats[trackerCat] is ok); if not found, do not overwrite category or trackerCategorySavePath and log/Info that the tracker category was not present in GetCategories to preserve the generic save path and avoid tagging with a non-existent qBittorrent category.
🧹 Nitpick comments (2)
web/src/pages/CrossSeedPage.tsx (1)
480-488: Consider adding delete confirmation for destructive action.The delete button immediately removes the mapping without confirmation. While inline CRUD often skips confirmation for speed, accidental deletion of a carefully configured mapping could be frustrating.
A lightweight option is an undo toast rather than a blocking dialog.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/CrossSeedPage.tsx` around lines 480 - 488, Add a lightweight confirmation/undo flow around the destructive delete action: instead of calling deleteMutation.mutate(mapping.indexerId) directly from the Button onClick in CrossSeedPage (the Button that renders the X icon), trigger a short-lived confirmation or dispatch the delete but show an undo toast with an expiration (e.g., call a wrapper like confirmOrUndoDelete(mapping.indexerId)). Implement the wrapper to either (a) open a small non-blocking confirm prompt before calling deleteMutation.mutate, or (b) optimistically call deleteMutation.mutate while storing the deleted item in state and displaying an undo toast that cancels/rolls back the delete if clicked within the timeout. Ensure the logic references deleteMutation and mapping.indexerId and that the toast/confirmation cleans up stored state on timeout or undo.internal/services/crossseed/crossseed_test.go (1)
1656-1658: The new tracker-category branch is still untested in this suite.Every row passes
niland""into the new parameters, soTestCrossSeed_CategoryAndTagPreservationstill never exercises the by-tracker override or the fallback precedence when no mapping exists. Please add at least one case for each of those paths here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/crossseed/crossseed_test.go` around lines 1656 - 1658, Add test rows to TestCrossSeed_CategoryAndTagPreservation that exercise the new tracker-category branch by passing a non-nil tracker parameter into the call to svc.determineCrossSeedCategory (instead of nil) and by providing a non-empty fallback string (instead of "") for the tracker key; specifically create one case where the settings contain a by-tracker mapping that should override the default mapping (verify baseCategory/crossCategory match the by-tracker mapping) and one case where no mapping exists so the fallback precedence is taken (verify the fallback category is returned). Use the existing table-driven test pattern and reference svc.determineCrossSeedCategory, the settings variable, tt.request and tt.matched to locate where to add these cases.
🤖 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/proxy/handler.go`:
- Around line 109-123: The NewHandler constructor signature was extended to
accept indexerCategoryStore; update every call site that constructs the proxy
Handler to pass the new argument (the CrossSeedIndexerCategoryStore instance).
Locate usages of NewHandler (e.g., where the proxy handler is created in server
wiring) and add the indexerCategoryStore variable to the argument list so the
call matches NewHandler(clientPool, clientAPIKeyStore, instanceStore,
syncManager, cache, svc, indexerCategoryStore, baseURL).
In `@internal/services/crossseed/service.go`:
- Around line 4647-4648: The current branch uses isTrackerCategoryMode (set from
instance.HardlinkDirPreset == "by-tracker") to decide the tracker-specific
AutoTMM path even when no tracker mapping actually resolved; change the logic to
use a new boolean produced during category resolution (e.g.,
trackerCategoryMatched) instead of isTrackerCategoryMode, and pass that flag
into the shouldEnableAutoTMM(...) call (replacing isTrackerCategoryMode) so
tracker-category AutoTMM is only considered when a tracker mapping was actually
matched; update the caller site around the tmmDecision assignment (which
currently calls shouldEnableAutoTMM(crossCategory, matchedTorrent.AutoManaged,
useCategoryFromIndexer, useCustomCategory, isTrackerCategoryMode,
actualCategorySavePath, props.SavePath)) to use trackerCategoryMatched.
In `@internal/services/crossseed/tracker_category.go`:
- Around line 123-129: The code currently swallows context cancellation by
returning ("", false) when store.List(ctx, instanceID) fails; change
ResolveTrackerCategory to detect and propagate context errors instead of
treating them as "no mapping": after the store.List error, check if
errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) (or
use ctx.Err()) and return the error to the caller (update ResolveTrackerCategory
signature to return an error and propagate that through its callers), otherwise
keep the existing debug log and return ("", false) for non-context errors; use
the errors package for comparisons and import context where needed.
In `@internal/services/dirscan/service.go`:
- Around line 1817-1827: The InjectRequest struct used in service.go is missing
the TrackerCategorySavePath field, causing an unknown-field compile error when
constructing the InjectRequest literal in that file; open the InjectRequest
definition (in inject.go) and add a TrackerCategorySavePath field with the same
type as trackerCategorySavePath used at the call site (match existing field
styles and struct tags), then rebuild to ensure the struct literal in service.go
(the injectReq construction) compiles cleanly.
In `@web/src/pages/CrossSeedPage.tsx`:
- Around line 471-479: The combobox currently ignores falsy selections so
clearing a category leaves backend unchanged; update the onChange handler for
CategoryCombobox to handle deselection by calling setMutation.mutate({
indexerId: mapping.indexerId, category: cat ?? '' }) (or null if your API
expects null) when cat is falsy so the backend is updated to clear the mapping;
alternatively, if you prefer preventing deselection, add an allowDeselect prop
to CategoryCombobox and pass allowDeselect={false} for existing mappings to
prevent toggling off.
---
Outside diff comments:
In `@internal/services/dirscan/service.go`:
- Around line 113-125: The production call site that constructs the directory
scanner service still uses the old 9-argument signature for NewService; update
the call to match the new signature by supplying the new indexerCategoryStore
argument (type *models.CrossSeedIndexerCategoryStore) — pass the actual
indexerCategoryStore variable from your startup wiring or nil if it is
intentionally optional; ensure the call to dirscan.NewService includes this
additional parameter so the constructor signature and call sites match.
---
Duplicate comments:
In `@internal/services/crossseed/tracker_category.go`:
- Around line 73-86: The current raw substring checks in
MatchAnnounceDomainToIndexer (the strings.Contains uses around normalizedDomain
and domainNorm) misroute short indexer names; change them to
token/boundary-aware comparisons: split normalizedDomain and domainNorm into
labels/tokens using separator normalization (call normalizeDomainNameValue and
split on non-alphanumerics or use strings.FieldsFunc) and then check for exact
token equality with normalizedIndexer (or compare host labels returned by
stripTrackerTLD) instead of using strings.Contains; update the two places that
currently call strings.Contains(normalizedDomain, normalizedIndexer) and
strings.Contains(domainNorm, normalizedIndexer) to iterate tokens and return
true only when a token == normalizedIndexer.
In `@internal/services/dirscan/service.go`:
- Around line 1796-1809: The code sets category = trackerCat even when qbitCats
doesn't contain trackerCat, leaving trackerCategorySavePath empty; change the
logic in the ResolveTrackerCategory handling so you only assign category =
trackerCat (and set trackerCategorySavePath = cat.SavePath) when qbitCats
contains trackerCat (i.e. the lookup qbitCats[trackerCat] is ok); if not found,
do not overwrite category or trackerCategorySavePath and log/Info that the
tracker category was not present in GetCategories to preserve the generic save
path and avoid tagging with a non-existent qBittorrent category.
---
Nitpick comments:
In `@internal/services/crossseed/crossseed_test.go`:
- Around line 1656-1658: Add test rows to
TestCrossSeed_CategoryAndTagPreservation that exercise the new tracker-category
branch by passing a non-nil tracker parameter into the call to
svc.determineCrossSeedCategory (instead of nil) and by providing a non-empty
fallback string (instead of "") for the tracker key; specifically create one
case where the settings contain a by-tracker mapping that should override the
default mapping (verify baseCategory/crossCategory match the by-tracker mapping)
and one case where no mapping exists so the fallback precedence is taken (verify
the fallback category is returned). Use the existing table-driven test pattern
and reference svc.determineCrossSeedCategory, the settings variable, tt.request
and tt.matched to locate where to add these cases.
In `@web/src/pages/CrossSeedPage.tsx`:
- Around line 480-488: Add a lightweight confirmation/undo flow around the
destructive delete action: instead of calling
deleteMutation.mutate(mapping.indexerId) directly from the Button onClick in
CrossSeedPage (the Button that renders the X icon), trigger a short-lived
confirmation or dispatch the delete but show an undo toast with an expiration
(e.g., call a wrapper like confirmOrUndoDelete(mapping.indexerId)). Implement
the wrapper to either (a) open a small non-blocking confirm prompt before
calling deleteMutation.mutate, or (b) optimistically call deleteMutation.mutate
while storing the deleted item in state and displaying an undo toast that
cancels/rolls back the delete if clicked within the timeout. Ensure the logic
references deleteMutation and mapping.indexerId and that the toast/confirmation
cleans up stored state on timeout or undo.
🪄 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: 563f4e23-121d-4281-be80-5c8e679f4266
📒 Files selected for processing (28)
cmd/qui/main.gointernal/api/handlers/crossseed.gointernal/api/handlers/dirscan_webhook_test.gointernal/api/server.gointernal/api/server_test.gointernal/database/migrations/069_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/crossseed_test.gointernal/services/crossseed/domain_aliases.gointernal/services/crossseed/domain_aliases_test.gointernal/services/crossseed/hardlink_mode_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/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 (8)
- web/src/types/index.ts
- internal/database/migrations/069_add_tracker_category_settings.sql
- internal/services/crossseed/domain_aliases.go
- internal/models/crossseed_indexer_categories_test.go
- web/src/lib/api.ts
- internal/proxy/crossseed_indexer_categories_test.go
- internal/api/handlers/crossseed.go
- internal/services/dirscan/inject.go
🚧 Files skipped from review as they are similar to previous changes (10)
- internal/services/dirscan/webhook_queue_test.go
- internal/api/server_test.go
- internal/services/dirscan/cancel_scan_test.go
- cmd/qui/main.go
- internal/api/server.go
- internal/proxy/handler_test.go
- internal/services/crossseed/hardlink_mode_test.go
- internal/services/crossseed/domain_aliases_test.go
- internal/models/crossseed_indexer_categories.go
- internal/web/swagger/openapi.yaml
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 (1)
internal/proxy/handler.go (1)
412-438:⚠️ Potential issue | 🟠 MajorRegister this route outside
prepareProxyContextMiddleware.
handleCrossSeedIndexerCategoriesonly needs the instance ID thatClientAPIKeyMiddlewarealready adds to the context. Mounted under Line 414, it now returns the generic proxy 502 wheneverprepareProxyContextcannot load the qBittorrent client or the instance is disabled, even though this endpoint never talks to qBittorrent.Suggested routing change
proxyRoute := httphelpers.JoinBasePath(h.basePath, "/proxy/{api-key}") proxyRouter := r.With(ClientAPIKeyMiddleware(h.clientAPIKeyStore)) + +proxyRouter.Get( + httphelpers.JoinBasePath(h.basePath, "/proxy/{api-key}/api/v2/cross-seed/indexer-categories"), + h.handleCrossSeedIndexerCategories, +) // Scoped proxy routes retain API key middleware and prepare proxy context proxyRouter.Route(proxyRoute, func(pr chi.Router) { // Apply proxy context middleware (adds instance info to context) pr.Use(h.prepareProxyContextMiddleware) @@ - // cross-seed extensions (qui-specific, not forwarded to qBittorrent) - pr.Get("/api/v2/cross-seed/indexer-categories", h.handleCrossSeedIndexerCategories) - // Handle the base proxy path and any nested paths requested through the proxy pr.HandleFunc("/", h.ServeHTTP) pr.HandleFunc("/*", h.ServeHTTP) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/proxy/handler.go` around lines 412 - 438, The route registration for handleCrossSeedIndexerCategories is currently nested inside the proxyRouter.Route block that applies prepareProxyContextMiddleware, causing requests to fail when prepareProxyContext cannot load a qBittorrent client; move the pr.Get("/api/v2/cross-seed/indexer-categories", h.handleCrossSeedIndexerCategories) call out of (i.e. register it before or in a separate route that does not use) prepareProxyContextMiddleware so it only uses the ClientAPIKeyMiddleware-provided instance ID and is not subject to the qBittorrent client checks in prepareProxyContextMiddleware.
🧹 Nitpick comments (1)
internal/api/handlers/crossseed.go (1)
1717-1730: Trim and reject blankcategoryvalues on PUT.
req.Categorygoes straight into the store, so" tv "and" "become distinct mappings. Since DELETE already exists to remove a mapping, I'd normalize here and fail fast on blank input instead of persisting it.Suggested validation
var req struct { IndexerID int `json:"indexerId"` Category string `json:"category"` } if err := json.NewDecoder(r.Body).Decode(&req); err != nil { RespondError(w, http.StatusBadRequest, "Invalid request body") return } if req.IndexerID <= 0 { RespondError(w, http.StatusBadRequest, "indexerId must be a positive integer") return } + category := strings.TrimSpace(req.Category) + if category == "" { + RespondError(w, http.StatusBadRequest, "category is required") + return + } - if err := h.indexerCategoryStore.Set(r.Context(), instanceID, req.IndexerID, req.Category); err != nil { + if err := h.indexerCategoryStore.Set(r.Context(), instanceID, req.IndexerID, category); err != nil { log.Error().Err(err).Int("instanceID", instanceID).Int("indexerID", req.IndexerID).Msg("Failed to set indexer category") RespondError(w, http.StatusInternalServerError, "Failed to save indexer category") return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handlers/crossseed.go` around lines 1717 - 1730, The handler currently stores req.Category verbatim, allowing values like " tv " or " " to be persisted; update the request validation in the same handler that decodes into the local req struct so that you trim whitespace (e.g. use strings.TrimSpace on req.Category) and if the trimmed value is empty respond with RespondError(w, http.StatusBadRequest, "category must be a non-empty string") rather than calling h.indexerCategoryStore.Set; ensure you pass the trimmed category into indexerCategoryStore.Set when calling it.
🤖 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/service.go`:
- Around line 11113-11118: The code determines destDir using categorySavePath
but later logic still uses the original categorySavePath rather than the derived
csPath/tracker-mapped path, causing the first hardlink add to skip the
tracker-category behavior; update the method to compute and use a single
effectiveSavePath (e.g., csPath or effectiveCategoryPath) and replace all
checks/usages of categorySavePath in the blocks around the branches that set
destDir (symbols: categorySavePath, csPath, destDir, isTrackerCategoryMode,
hardlinktree.HasCommonRootFolder, pathutil.IsolationFolderName) so the first
hardlink add and the other referenced blocks (around the other occurrences you
noted) use the derived tracker-category path consistently. Ensure you propagate
that effectiveSavePath into the autoTMM=false branch and any later logic that
previously referenced categorySavePath.
- Around line 10055-10056: The current check that does "if cat, found,
resolveErr := ResolveTrackerCategory(...); resolveErr != nil { return \"\",
\"\", false }" discards category info on transient lookup errors; instead, log
the resolveErr (using the service logger, e.g., s.logger or the existing logging
utility) and fall through into the existing fallback path so the code can still
use any mapped category or the documented global fallback; specifically, change
handling around ResolveTrackerCategory to log resolveErr and not return early,
allowing the subsequent logic that uses cat and found (and the global fallback)
to execute.
In `@internal/services/dirscan/inject.go`:
- Around line 407-423: applyInversePathMapping currently checks for a '/'
boundary which fails on Windows; update the boundary check to use the OS path
separator instead (e.g. replace strings.HasPrefix(suffix, "/") with
strings.HasPrefix(suffix, string(os.PathSeparator)) or compare suffix[0] to
filepath.Separator) so that the function correctly recognizes path boundaries
across platforms; keep the existing prefix normalisation and final
filepath.Clean(searcheePath + suffix) behavior and ensure os or filepath is
imported if not already.
In `@internal/web/swagger/openapi.yaml`:
- Around line 4042-4061: The OpenAPI operation for the GET
/proxy/{api-key}/api/v2/cross-seed/indexer-categories is incorrectly inheriting
file-level ApiKeyAuth/SessionAuth; update the operation object (the "get" under
the /proxy/{api-key}/api/v2/cross-seed/indexer-categories path) to explicitly
disable inherited security by adding security: [] so the endpoint is documented
as authenticated only via the api-key path parameter (matching the existing
/proxy/{api-key}/api/v2/torrents/mediainfo pattern).
---
Outside diff comments:
In `@internal/proxy/handler.go`:
- Around line 412-438: The route registration for
handleCrossSeedIndexerCategories is currently nested inside the
proxyRouter.Route block that applies prepareProxyContextMiddleware, causing
requests to fail when prepareProxyContext cannot load a qBittorrent client; move
the pr.Get("/api/v2/cross-seed/indexer-categories",
h.handleCrossSeedIndexerCategories) call out of (i.e. register it before or in a
separate route that does not use) prepareProxyContextMiddleware so it only uses
the ClientAPIKeyMiddleware-provided instance ID and is not subject to the
qBittorrent client checks in prepareProxyContextMiddleware.
---
Nitpick comments:
In `@internal/api/handlers/crossseed.go`:
- Around line 1717-1730: The handler currently stores req.Category verbatim,
allowing values like " tv " or " " to be persisted; update the request
validation in the same handler that decodes into the local req struct so that
you trim whitespace (e.g. use strings.TrimSpace on req.Category) and if the
trimmed value is empty respond with RespondError(w, http.StatusBadRequest,
"category must be a non-empty string") rather than calling
h.indexerCategoryStore.Set; ensure you pass the trimmed category into
indexerCategoryStore.Set when calling it.
🪄 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: f7c99d69-1df7-4fb6-8e3b-780d19577304
📒 Files selected for processing (28)
cmd/qui/main.gointernal/api/handlers/crossseed.gointernal/api/handlers/dirscan_webhook_test.gointernal/api/server.gointernal/api/server_test.gointernal/database/migrations/069_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/crossseed_test.gointernal/services/crossseed/domain_aliases.gointernal/services/crossseed/domain_aliases_test.gointernal/services/crossseed/hardlink_mode_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/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/handlers/dirscan_webhook_test.go
- internal/services/crossseed/domain_aliases_test.go
- internal/services/crossseed/domain_aliases.go
- internal/database/migrations/069_add_tracker_category_settings.sql
- internal/services/crossseed/tracker_category_test.go
- internal/services/dirscan/inject_test.go
- internal/proxy/crossseed_indexer_categories_test.go
- internal/services/crossseed/crossseed_test.go
- internal/models/crossseed_indexer_categories_test.go
- internal/models/crossseed_indexer_categories.go
- web/src/lib/api.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/services/dirscan/webhook_queue_test.go
- internal/services/dirscan/cancel_scan_test.go
- internal/services/crossseed/hardlink_mode_test.go
- internal/services/crossseed/auto_tmm_test.go
- internal/services/crossseed/tracker_category.go
- cmd/qui/main.go
- web/src/pages/CrossSeedPage.tsx
- web/src/types/index.ts
CjE5
force-pushed
the
develop
branch
2 times, most recently
from
April 5, 2026 01:36
aafe20b to
dd834c6
Compare
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
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 (1)
internal/services/crossseed/service.go (1)
8891-8907:⚠️ Potential issue | 🟠 MajorRemove the unconditional specific-domain match.
normalizedIndexerName == normalizedSpecificDomainnever checks the currentdomain, so once it evaluates true, any torrent with at least one tracker domain is treated as coming from this indexer. That can incorrectly filter out unrelated indexers during seeded search.💡 Suggested fix
if normalizedDomain == normalizedSpecificDomain { log.Debug(). Str("matchType", "specific_indexer_domain_direct"). Str("torrentDomain", domain). Str("indexerDomain", specificIndexerDomain). Str("indexerName", indexerName). Msg("[CROSSSEED-DOMAIN] *** MATCH FOUND - Specific indexer domain direct match ***") return true } - if normalizedIndexerName == normalizedSpecificDomain { - log.Debug(). - Str("matchType", "indexer_name_to_specific_domain"). - Str("torrentDomain", domain). - Str("indexerDomain", specificIndexerDomain). - Str("indexerName", indexerName). - Msg("[CROSSSEED-DOMAIN] *** MATCH FOUND - Indexer name matches specific domain ***") - return true - } if strings.Contains(normalizedDomain, normalizedSpecificDomain) || strings.Contains(normalizedSpecificDomain, normalizedDomain) { log.Debug(). Str("matchType", "specific_indexer_domain_partial").🤖 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 8891 - 8907, The second unconditional specific-domain match (the if checking normalizedIndexerName == normalizedSpecificDomain) incorrectly ignores the current torrent domain and should be removed or tightened; modify that branch so it only returns true when the specific domain also matches the current tracker domain (e.g., require normalizedDomain == normalizedSpecificDomain in addition to comparing normalizedIndexerName), or delete the entire if-block referencing normalizedIndexerName == normalizedSpecificDomain so only the explicit domain match (normalizedDomain == normalizedSpecificDomain) is used; update the log and return accordingly for the remaining valid checks (references: normalizedIndexerName, normalizedSpecificDomain, normalizedDomain).
🧹 Nitpick comments (3)
web/src/pages/CrossSeedPage.tsx (1)
421-429: Fetch only the metadata this editor actually uses.This component only reads
instanceMeta.categories, sogetTagsandgetInstancePreferencesadd two unused requests every time the section expands.♻️ Suggested cleanup
- const { data: instanceMeta } = useQuery({ - queryKey: ["instance-metadata", instanceId], - queryFn: async () => { - const [categories, tags, preferences] = await Promise.all([ - api.getCategories(instanceId), - api.getTags(instanceId), - api.getInstancePreferences(instanceId), - ]) - return { categories, tags, preferences } - }, + const { data: categories } = useQuery({ + queryKey: ["instance-categories", instanceId], + queryFn: () => api.getCategories(instanceId), staleTime: 60000, gcTime: 1800000, enabled: isExpanded, }) const categoryOptions = useMemo( - () => buildCategorySelectOptions(instanceMeta?.categories ?? {}), - [instanceMeta] + () => buildCategorySelectOptions(categories ?? {}), + [categories] )Also applies to: 436-439
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/CrossSeedPage.tsx` around lines 421 - 429, The useQuery call building instanceMeta currently requests api.getCategories, api.getTags, and api.getInstancePreferences but the component only uses instanceMeta.categories; update the queryFn inside the useQuery (queryKey ["instance-metadata", instanceId]) to only await and return api.getCategories(instanceId) (wrap result as { categories }) and remove calls to api.getTags and api.getInstancePreferences; apply the same change to the other identical useQuery occurrence that also requests tags/preferences so only getCategories is fetched and returned as { categories } for the component to consume.internal/services/dirscan/service.go (1)
1791-1808: Consider snapshotting the preset/category metadata outside this hot path.
tryMatchAndInjectnow re-reads the instance row and qBittorrent categories for every accepted candidate. On larger scans that adds avoidable store/cache traffic, and it can make one run switch behavior mid-flight if the instance settings change while the scan is still running.🤖 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 1791 - 1808, The code inside tryMatchAndInject repeatedly calls s.instanceStore.Get and s.syncManager.GetCategories (guarded by s.indexerCategoryStore) for every accepted candidate; snapshot the instance preset and qBittorrent category map once before the hot loop instead of re-reading on each candidate. Specifically, move the instance lookup (instanceStore.Get for dir.TargetInstanceID) and the category resolution logic (crossseed.ResolveTrackerCategory and s.syncManager.GetCategories) out of the per-candidate path into a single precomputed set of local values (e.g., instancePreset/HardlinkDirPreset, trackerCategory, and qbitCats) keyed by instance ID or computed once per tryMatchAndInject call, then use those local variables (instead of calling instanceStore.Get and s.syncManager.GetCategories repeatedly) inside the existing branch that references instance.HardlinkDirPreset, trackerCat, and trackerCategorySavePath.internal/services/dirscan/inject.go (1)
522-538: Eliminate duplicate inverse path mapping logic.The inverse path mapping for
TrackerCategorySavePathis computed twice: first at lines 493-498 (stored ineffectiveBaseDir) and again here at lines 528-534 (stored inselectedBaseDir). Whenreq.TrackerCategorySavePath != "", both computations produce identical results.Since
effectiveBaseDiralready holds the correctly mapped value in tracker-category mode, you can reuse it directly.♻️ Proposed refactor
if req.TrackerCategorySavePath != "" { - // Tracker-category mode: files must land directly inside the category save - // path — no isolation subfolder — so AutoTMM can manage placement. - // TrackerCategorySavePath is qBittorrent's view of the path; translate it - // to a host filesystem path when QbitPathPrefix is configured. - selectedBaseDir = req.TrackerCategorySavePath - if req.QbitPathPrefix != "" && req.Searchee != nil { - mapped, ok := applyInversePathMapping(selectedBaseDir, filepath.Dir(req.Searchee.Path), req.QbitPathPrefix) - if !ok { - return nil, "", fmt.Errorf("tracker category save path %q cannot be mapped to host filesystem: QbitPathPrefix %q does not match", selectedBaseDir, req.QbitPathPrefix) - } - selectedBaseDir = mapped - } + // Tracker-category mode: files must land directly inside the category + // save path — no isolation subfolder — so AutoTMM can manage placement. + // effectiveBaseDir was already computed from TrackerCategorySavePath + // (with inverse path mapping applied if needed) at the top of this + // function. + selectedBaseDir = effectiveBaseDir if err := os.MkdirAll(selectedBaseDir, 0o755); err != nil { return nil, "", fmt.Errorf("create tracker category dir %q: %w", selectedBaseDir, err) } destDir = selectedBaseDir } else {🤖 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 522 - 538, The duplicate inverse mapping for tracker-category mode should be removed by reusing the previously computed effectiveBaseDir instead of recomputing it into selectedBaseDir; when req.TrackerCategorySavePath != "" use effectiveBaseDir (which was already derived via applyInversePathMapping using req.QbitPathPrefix and req.Searchee) for mkdir and destDir assignment, and drop the second applyInversePathMapping block and its error branch to avoid redundant logic while preserving the os.MkdirAll call and error handling for the chosen directory.
🤖 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/crossseed_indexer_categories.go`:
- Around line 75-87: Trim the indexerName once and use that trimmed value for
both the empty check and the DB lookup: create a local variable (e.g.,
trimmedName := strings.TrimSpace(indexerName)), return early if trimmedName ==
"", and pass trimmedName into s.db.QueryRowContext(ctx, query, instanceID,
trimmedName) instead of the original indexerName so inputs like " IndexerA "
match existing mappings.
In `@internal/services/crossseed/service.go`:
- Around line 10083-10099: The current fallback calls
s.indexerCategoryStore.GetByIndexerName whenever req.IndexerName is set, which
can override a real tracker when announceDomain was present but unmapped; modify
the logic in the function that contains this block so that you only invoke
GetByIndexerName when the parsed announceDomain is empty/absent (e.g., check
that announceDomain == "" or nil) in addition to req != nil && req.IndexerName
!= ""; keep the existing logging and return behavior (use instance.ID,
req.IndexerName, mappedCategory) but skip the indexer-name lookup when an
announceDomain exists so the code falls through to the global category rules
instead.
---
Outside diff comments:
In `@internal/services/crossseed/service.go`:
- Around line 8891-8907: The second unconditional specific-domain match (the if
checking normalizedIndexerName == normalizedSpecificDomain) incorrectly ignores
the current torrent domain and should be removed or tightened; modify that
branch so it only returns true when the specific domain also matches the current
tracker domain (e.g., require normalizedDomain == normalizedSpecificDomain in
addition to comparing normalizedIndexerName), or delete the entire if-block
referencing normalizedIndexerName == normalizedSpecificDomain so only the
explicit domain match (normalizedDomain == normalizedSpecificDomain) is used;
update the log and return accordingly for the remaining valid checks
(references: normalizedIndexerName, normalizedSpecificDomain, normalizedDomain).
---
Nitpick comments:
In `@internal/services/dirscan/inject.go`:
- Around line 522-538: The duplicate inverse mapping for tracker-category mode
should be removed by reusing the previously computed effectiveBaseDir instead of
recomputing it into selectedBaseDir; when req.TrackerCategorySavePath != "" use
effectiveBaseDir (which was already derived via applyInversePathMapping using
req.QbitPathPrefix and req.Searchee) for mkdir and destDir assignment, and drop
the second applyInversePathMapping block and its error branch to avoid redundant
logic while preserving the os.MkdirAll call and error handling for the chosen
directory.
In `@internal/services/dirscan/service.go`:
- Around line 1791-1808: The code inside tryMatchAndInject repeatedly calls
s.instanceStore.Get and s.syncManager.GetCategories (guarded by
s.indexerCategoryStore) for every accepted candidate; snapshot the instance
preset and qBittorrent category map once before the hot loop instead of
re-reading on each candidate. Specifically, move the instance lookup
(instanceStore.Get for dir.TargetInstanceID) and the category resolution logic
(crossseed.ResolveTrackerCategory and s.syncManager.GetCategories) out of the
per-candidate path into a single precomputed set of local values (e.g.,
instancePreset/HardlinkDirPreset, trackerCategory, and qbitCats) keyed by
instance ID or computed once per tryMatchAndInject call, then use those local
variables (instead of calling instanceStore.Get and s.syncManager.GetCategories
repeatedly) inside the existing branch that references
instance.HardlinkDirPreset, trackerCat, and trackerCategorySavePath.
In `@web/src/pages/CrossSeedPage.tsx`:
- Around line 421-429: The useQuery call building instanceMeta currently
requests api.getCategories, api.getTags, and api.getInstancePreferences but the
component only uses instanceMeta.categories; update the queryFn inside the
useQuery (queryKey ["instance-metadata", instanceId]) to only await and return
api.getCategories(instanceId) (wrap result as { categories }) and remove calls
to api.getTags and api.getInstancePreferences; apply the same change to the
other identical useQuery occurrence that also requests tags/preferences so only
getCategories is fetched and returned as { categories } for the component to
consume.
🪄 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: 99636e17-20aa-4388-8350-6e4154f38024
📒 Files selected for processing (28)
cmd/qui/main.gointernal/api/handlers/crossseed.gointernal/api/handlers/dirscan_webhook_test.gointernal/api/server.gointernal/api/server_test.gointernal/database/migrations/069_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/crossseed_test.gointernal/services/crossseed/domain_aliases.gointernal/services/crossseed/domain_aliases_test.gointernal/services/crossseed/hardlink_mode_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/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 (7)
- internal/api/handlers/dirscan_webhook_test.go
- web/src/types/index.ts
- internal/services/crossseed/domain_aliases.go
- internal/database/migrations/069_add_tracker_category_settings.sql
- internal/services/crossseed/tracker_category_test.go
- internal/models/crossseed_indexer_categories_test.go
- internal/proxy/crossseed_indexer_categories_test.go
🚧 Files skipped from review as they are similar to previous changes (14)
- internal/proxy/handler_test.go
- internal/services/dirscan/webhook_queue_test.go
- internal/services/crossseed/domain_aliases_test.go
- internal/services/crossseed/auto_tmm_test.go
- internal/services/dirscan/cancel_scan_test.go
- cmd/qui/main.go
- internal/services/dirscan/inject_test.go
- web/src/lib/api.ts
- internal/services/crossseed/tracker_category.go
- internal/services/crossseed/hardlink_mode_test.go
- internal/proxy/handler.go
- internal/api/server.go
- internal/api/handlers/crossseed.go
- internal/web/swagger/openapi.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
web/src/pages/CrossSeedPage.tsx (1)
423-429: Only fetch categories for this panel.The only downstream consumer here is
buildCategorySelectOptions(instanceMeta?.categories ?? {}). Pulling tags and preferences adds two extra round-trips every time the section expands and makes category suggestions depend on unrelated endpoints.♻️ Suggested cleanup
const { data: instanceMeta } = useQuery({ queryKey: ["instance-metadata", instanceId], queryFn: async () => { - const [categories, tags, preferences] = await Promise.all([ - api.getCategories(instanceId), - api.getTags(instanceId), - api.getInstancePreferences(instanceId), - ]) - return { categories, tags, preferences } + const categories = await api.getCategories(instanceId) + return { categories } }, staleTime: 60000, gcTime: 1800000, enabled: isExpanded, })Also applies to: 436-439
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/CrossSeedPage.tsx` around lines 423 - 429, The query function currently fetches categories, tags, and preferences but only categories are used downstream (see buildCategorySelectOptions and instanceMeta?.categories), causing unnecessary network calls; change the queryFn to only call api.getCategories(instanceId) (remove api.getTags and api.getInstancePreferences) and return just categories so category suggestions depend only on api.getCategories and not unrelated endpoints (apply the same change to the other occurrence around the 436-439 block).internal/services/dirscan/service.go (1)
1792-1805: Move invariant instance/category lookups out of the per-match hot path.This branch re-fetches the same instance row and qBittorrent category map for every accepted candidate. On larger scans that becomes a lot of repeated DB/API round-trips for data that is constant for the whole run.
🤖 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 1792 - 1805, The code repeatedly calls s.instanceStore.Get, checks instance.HardlinkDirPreset == "by-tracker", calls crossseed.ResolveTrackerCategory and s.syncManager.GetCategories inside the per-candidate match path; move those invariant lookups out of the per-match hot path by fetching the instance once (via s.instanceStore.Get for dir.TargetInstanceID), evaluating HardlinkDirPreset, resolving trackerCat once via crossseed.ResolveTrackerCategory, and fetching qbitCats once via s.syncManager.GetCategories before iterating candidates, then use the cached instance, trackerCat, and qbitCats inside the match logic and remove the repeated calls (keep existing error handling but abort or skip early based on the cached results).
🤖 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/crossseed_indexer_categories.go`:
- Around line 99-108: The Set method currently trims category but still writes
empty strings, causing inconsistent behavior with GetByIndexerName and List;
modify CrossSeedIndexerCategoryStore.Set to trim the category
(strings.TrimSpace) and if the result is empty either return a validation error
(reject write) or perform a DELETE for that (instanceID, indexerID) instead of
inserting/updating; update any callers and request validation/OpenAPI to enforce
the same non-blank rule so saved state and lookups remain aligned (refer to Set,
GetByIndexerName and List to ensure consistent behavior).
In `@internal/services/crossseed/service.go`:
- Around line 8900-8906: The current fallback uses
strings.Contains(normalizedDomain, normalizedSpecificDomain) which causes false
positives (e.g., "notredacted.sh" matching "redacted.sh"); update the logic in
the specific-indexer domain check (the block using normalizedDomain and
normalizedSpecificDomain and logging via log.Debug) to only treat domains as
matching when they are equal or when one is a proper subdomain/dot-suffix of the
other (e.g., domain == specificDomain OR strings.HasSuffix(normalizedDomain,
"."+normalizedSpecificDomain) OR strings.HasSuffix(normalizedSpecificDomain,
"."+normalizedDomain)); replace the strings.Contains(...) conditions accordingly
so unrelated hostnames are not treated as matches.
- Around line 11114-11117: effectiveCategorySavePath is being set whenever
crossCategory != "" even if no tracker mapping matched (isTrackerCategoryMode ==
false); change the logic so that buildCategorySavePath(...) is only used to
produce a tracker-specific save path when a tracker mapping actually resolved.
Specifically, only assign effectiveCategorySavePath from
s.buildCategorySavePath(ctx, instance, selectedBaseDir, incomingTrackerDomain,
candidate, req) when isTrackerCategoryMode (or the internal mapping-match
condition) is true for this add; keep any tracker subdirectory in transient
variables used for destDir/savepath for the current operation, but do not pass a
tracker-derived path into ensureCrossCategory(...) or other places that persist
category presets unless a mapping matched. Update all occurrences around
effectiveCategorySavePath (including the similar block at the later occurrence
mentioned) to follow this conditional assignment approach.
In `@internal/services/crossseed/tracker_category.go`:
- Around line 64-76: normalizeTrackerIndexerName can return an empty string
which makes strings.Contains(normalizedDomain, normalizedIndexer) trivially
true; after computing normalizedIndexer, add a guard that if normalizedIndexer
== "" you do not consider this mapping a match (i.e., skip the partial/direct
match logic or immediately return/continue false for this mapping).
Specifically, check normalizedIndexer right after the call to
normalizeTrackerIndexerName and ensure you do not run the strings.Contains check
(and do not treat an empty normalizedIndexer as a match) so normalizedDomain and
normalizedIndexer comparisons only happen when normalizedIndexer is non-empty.
In `@internal/services/dirscan/inject.go`:
- Around line 421-428: The child-path match fails when cleanPrefix is a
filesystem root because code always appends a separator (prefixWithSep :=
cleanPrefix + string(filepath.Separator)), producing a double-separator that
doesn't match cleanPath; update the construction of prefixWithSep to avoid
adding an extra separator when cleanPrefix already ends with filepath.Separator
(i.e., use strings.HasSuffix(cleanPrefix, string(filepath.Separator)) to choose
between cleanPrefix and cleanPrefix+separator), then keep the existing HasPrefix
check, TrimPrefix and filepath.Join logic (references: cleanPrefix,
prefixWithSep, cleanPath, strings.HasPrefix, strings.TrimPrefix, cleanHostBase,
filepath.Join).
In `@internal/services/dirscan/service.go`:
- Around line 1791-1796: The by-tracker override must be gated on the instance
actually being in hardlink/reflink active mode; update the branch that currently
only checks instance.HardlinkDirPreset == "by-tracker" (the code around
s.instanceStore.Get(...) and the call to crossseed.ResolveTrackerCategory(...))
to require the instance is actively in hardlink mode as well (e.g.
instance.HardlinkDirPreset == "by-tracker" &&
instance.<active-link-field/method> indicates hardlink mode). Make the same
change in the other similar block (lines 1822-1832) so both the
ResolveTrackerCategory call and the logic that forwards TrackerCategorySavePath
are skipped unless the instance reports active link mode.
In `@web/src/pages/CrossSeedPage.tsx`:
- Around line 415-419: The mappings query currently defaults to [] which hides
loading/error states; update the useQuery call that fetches
api.getCrossSeedIndexerCategories(instanceId) (queryKey
["cross-seed-indexer-categories", instanceId], enabled: isExpanded) to also
destructure the status flags (e.g. isLoading and isError or isFetching and
isError) and use those to render explicit loading/error UI and to disable the
"Add mapping" button until the query has resolved; specifically, change the Add
mapping button component to set disabled={isLoading || isError} (or similar) and
show a spinner or error message when the query flags indicate pending or failed
state.
---
Nitpick comments:
In `@internal/services/dirscan/service.go`:
- Around line 1792-1805: The code repeatedly calls s.instanceStore.Get, checks
instance.HardlinkDirPreset == "by-tracker", calls
crossseed.ResolveTrackerCategory and s.syncManager.GetCategories inside the
per-candidate match path; move those invariant lookups out of the per-match hot
path by fetching the instance once (via s.instanceStore.Get for
dir.TargetInstanceID), evaluating HardlinkDirPreset, resolving trackerCat once
via crossseed.ResolveTrackerCategory, and fetching qbitCats once via
s.syncManager.GetCategories before iterating candidates, then use the cached
instance, trackerCat, and qbitCats inside the match logic and remove the
repeated calls (keep existing error handling but abort or skip early based on
the cached results).
In `@web/src/pages/CrossSeedPage.tsx`:
- Around line 423-429: The query function currently fetches categories, tags,
and preferences but only categories are used downstream (see
buildCategorySelectOptions and instanceMeta?.categories), causing unnecessary
network calls; change the queryFn to only call api.getCategories(instanceId)
(remove api.getTags and api.getInstancePreferences) and return just categories
so category suggestions depend only on api.getCategories and not unrelated
endpoints (apply the same change to the other occurrence around the 436-439
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: 132a15b7-9dee-4364-b4b0-e38c1e67873a
📒 Files selected for processing (28)
cmd/qui/main.gointernal/api/handlers/crossseed.gointernal/api/handlers/dirscan_webhook_test.gointernal/api/server.gointernal/api/server_test.gointernal/database/migrations/069_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/crossseed_test.gointernal/services/crossseed/domain_aliases.gointernal/services/crossseed/domain_aliases_test.gointernal/services/crossseed/hardlink_mode_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/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 (7)
- internal/api/server_test.go
- internal/services/crossseed/hardlink_mode_test.go
- internal/database/migrations/069_add_tracker_category_settings.sql
- internal/services/crossseed/tracker_category_test.go
- internal/models/crossseed_indexer_categories_test.go
- web/src/lib/api.ts
- internal/proxy/crossseed_indexer_categories_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/services/dirscan/cancel_scan_test.go
- internal/api/handlers/dirscan_webhook_test.go
- internal/services/dirscan/webhook_queue_test.go
- web/src/types/index.ts
- internal/api/server.go
- cmd/qui/main.go
- internal/services/crossseed/domain_aliases.go
- internal/services/crossseed/domain_aliases_test.go
- internal/api/handlers/crossseed.go
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 (1)
internal/services/crossseed/service.go (1)
4433-4461:⚠️ Potential issue | 🟠 MajorResolve the assigned category’s real
save_pathbefore using it downstream.When
crossCategorydiffers frombaseCategory(for example with a.crossaffix) and only the assigned category already has an explicit qBittorrent save path, this block still leavesactualCategorySavePathempty because it only inspectsbaseCategory. The regular add path then suppresses tracker-mode AutoTMM, and hardlink mode can prebuild files under a synthesized path that does not match the category qBittorrent will actually manage. Prefer the existingcrossCategorysave path when that category already exists, and usebaseCategory’s path only as the creation fallback.Based on learnings:
ensureCrossCategoryintentionally keeps warn-only behavior when an existing category’s save_path differs from the desired path and still caches it in createdCategories; callers should not treat this as categoryCreationFailed nor attempt repair.🤖 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 4433 - 4461, When resolving the save path for a cross-seed category ensure you check the actual configured save_path for crossCategory first (via s.syncManager.GetCategories(candidate.InstanceID)) and assign that to actualCategorySavePath if present, falling back to baseCategory only when crossCategory has no explicit SavePath; keep categorySavePath fallback to props.SavePath for later use, but pass actualCategorySavePath (which may now be the crossCategory path) into s.ensureCrossCategory, and do not treat a differing existing save_path as a creation failure (preserve the existing warn-only behavior and createdCategories caching).
♻️ Duplicate comments (2)
internal/services/crossseed/tracker_category.go (1)
78-90:⚠️ Potential issue | 🟠 MajorContainment matching is still broad enough to misroute categories.
strings.Containsmatching here can still produce false positives for short/common indexer tokens and return the wrong per-tracker category when multiple mappings exist. Please tighten this to boundary-aware matching (or equivalent stricter criteria) rather than raw substring checks.🤖 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 78 - 90, The current substring checks using strings.Contains on normalizedDomain and domainNorm are too permissive; replace them with boundary-aware token matching by splitting the normalized strings on common separators (e.g., '-', '.', '_') or using regex word-boundary matching so you only match complete tokens, not arbitrary substrings. Update the logic around normalizedDomain, normalizedIndexer, normalizeDomainNameValue and stripTrackerTLD to: produce token lists for both values, then check for exact token equality (or token-prefixed-with-separator) rather than raw contains; ensure both the initial domain check and the TLD-stripped domainNorm checks use this stricter token-equality/matched-boundary approach to avoid false positives.internal/services/crossseed/service.go (1)
8919-8923:⚠️ Potential issue | 🟠 MajorDon’t reintroduce substring matches in the TLD-stripped fallback.
strings.Containsstill makes unrelated domains look equivalent after normalization (notredactedvsredacted, etc.). That can exclude the wrong indexer or resolve the wrong tracker mapping from this fallback path. Keep this branch token/label-aware as well, not raw substring-based.🤖 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 8919 - 8923, The current TLD-stripped fallback uses raw substring checks between domainWithoutTLD and normalizedIndexerDomainName which can falsely match unrelated names; instead perform a label/token-aware comparison: tokenize both domainWithoutTLD and normalizedIndexerDomainName by label (split on '.'), then check equality or label-boundary containment (e.g., exact token sequence equality or one being a suffix/prefix of the other's label slice) rather than strings.Contains. Update the branch that uses domainWithoutTLD and normalizedIndexerDomainName (around normalizeDomainNameValue(stripTrackerTLD(...)) and s.normalizeDomainName(...)) to compare token arrays so matches require whole-label alignment.
🤖 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/pages/CrossSeedPage.tsx`:
- Around line 411-413: When dismissing the draft mapping (both the success/save
path and the cancel handler) also reset the popover flags so a new draft row
doesn't mount with popovers open: alongside clearing addingRow via
setAddingRow(null) call setAddIndexerOpen(false) and setOpenCategoryFor(null).
Update the handlers that currently only call setAddingRow (referencing
addingRow, setAddingRow) to also call setAddIndexerOpen(false) and
setOpenCategoryFor(null); apply the same fix in the analogous block around the
code referenced at lines ~541-555.
- Around line 421-439: The query is fetching categories, tags and preferences
together with Promise.all which causes the whole useQuery (queryKey
["instance-metadata", instanceId]) to fail if getTags or getInstancePreferences
errors and thus clears category suggestions; change the queryFn in the useQuery
that populates instanceMeta to only call api.getCategories(instanceId) (or
create a new queryKey like ["instance-categories", instanceId]) so
buildCategorySelectOptions(instanceMeta?.categories ?? {}) and the
categoryOptions useMemo only depend on the categories fetch; remove or move
api.getTags and api.getInstancePreferences out of this query (or fetch them in
separate queries with their own error handling) so a failure in tags/preferences
does not clear categories.
- Around line 351-392: The create option is currently inside CommandEmpty so it
only appears when there are zero matches and is not keyboard-selectable; move
the create UI into a dedicated CommandItem (e.g., add a CommandItem for the
trimmed search value) so it appears alongside partial matches and participates
in cmdk keyboard navigation. Specifically, render a CommandItem when
search.trim() is non-empty and not equal to an existing name, and wire its
onSelect to call onChange(search.trim()), onOpenChange(false) and setSearch("")
(same behavior as the current div), keeping the existing Plus icon and label;
leave CommandEmpty for the "No categories." message only.
---
Outside diff comments:
In `@internal/services/crossseed/service.go`:
- Around line 4433-4461: When resolving the save path for a cross-seed category
ensure you check the actual configured save_path for crossCategory first (via
s.syncManager.GetCategories(candidate.InstanceID)) and assign that to
actualCategorySavePath if present, falling back to baseCategory only when
crossCategory has no explicit SavePath; keep categorySavePath fallback to
props.SavePath for later use, but pass actualCategorySavePath (which may now be
the crossCategory path) into s.ensureCrossCategory, and do not treat a differing
existing save_path as a creation failure (preserve the existing warn-only
behavior and createdCategories caching).
---
Duplicate comments:
In `@internal/services/crossseed/service.go`:
- Around line 8919-8923: The current TLD-stripped fallback uses raw substring
checks between domainWithoutTLD and normalizedIndexerDomainName which can
falsely match unrelated names; instead perform a label/token-aware comparison:
tokenize both domainWithoutTLD and normalizedIndexerDomainName by label (split
on '.'), then check equality or label-boundary containment (e.g., exact token
sequence equality or one being a suffix/prefix of the other's label slice)
rather than strings.Contains. Update the branch that uses domainWithoutTLD and
normalizedIndexerDomainName (around
normalizeDomainNameValue(stripTrackerTLD(...)) and s.normalizeDomainName(...))
to compare token arrays so matches require whole-label alignment.
In `@internal/services/crossseed/tracker_category.go`:
- Around line 78-90: The current substring checks using strings.Contains on
normalizedDomain and domainNorm are too permissive; replace them with
boundary-aware token matching by splitting the normalized strings on common
separators (e.g., '-', '.', '_') or using regex word-boundary matching so you
only match complete tokens, not arbitrary substrings. Update the logic around
normalizedDomain, normalizedIndexer, normalizeDomainNameValue and
stripTrackerTLD to: produce token lists for both values, then check for exact
token equality (or token-prefixed-with-separator) rather than raw contains;
ensure both the initial domain check and the TLD-stripped domainNorm checks use
this stricter token-equality/matched-boundary approach to avoid false positives.
🪄 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: 73f9b427-24cf-4289-b836-e504cb94eee4
📒 Files selected for processing (28)
cmd/qui/main.gointernal/api/handlers/crossseed.gointernal/api/handlers/dirscan_webhook_test.gointernal/api/server.gointernal/api/server_test.gointernal/database/migrations/069_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/crossseed_test.gointernal/services/crossseed/domain_aliases.gointernal/services/crossseed/domain_aliases_test.gointernal/services/crossseed/hardlink_mode_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/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 (8)
- internal/api/server_test.go
- internal/services/crossseed/domain_aliases.go
- internal/database/migrations/069_add_tracker_category_settings.sql
- internal/services/crossseed/tracker_category_test.go
- internal/models/crossseed_indexer_categories_test.go
- internal/proxy/crossseed_indexer_categories_test.go
- web/src/types/index.ts
- internal/services/dirscan/inject_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- internal/services/dirscan/cancel_scan_test.go
- cmd/qui/main.go
- internal/api/handlers/dirscan_webhook_test.go
- internal/services/crossseed/domain_aliases_test.go
- internal/services/crossseed/hardlink_mode_test.go
- internal/services/crossseed/crossseed_test.go
- internal/services/dirscan/webhook_queue_test.go
- internal/proxy/handler.go
- internal/services/dirscan/inject.go
- internal/api/handlers/crossseed.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
internal/services/crossseed/service.go (2)
10041-10043:⚠️ Potential issue | 🟡 MinorHandle the nil-settings load failure explicitly here too.
This helper still swallows
GetAutomationSettings()errors, so callers that rely on the fallback load can silently skip custom/indexer/affix category rules and fall back to reuse semantics. At minimum, log and default tomodels.DefaultCrossSeedAutomationSettings()instead of leavingsettingsnil.💡 Possible fix
// Load settings if not provided by caller if settings == nil { - settings, _ = s.GetAutomationSettings(ctx) + loaded, err := s.GetAutomationSettings(ctx) + if err != nil { + log.Warn().Err(err). + Msg("[CROSSSEED] Failed to load automation settings for category resolution, using defaults") + settings = models.DefaultCrossSeedAutomationSettings() + } else { + settings = loaded + } }As per coding guidelines: “Prefer explicit error handling over silent failures in Go.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/crossseed/service.go` around lines 10041 - 10043, When settings == nil after calling s.GetAutomationSettings(ctx) we must not silently ignore the error; update the block that currently does "if settings == nil { settings, _ = s.GetAutomationSettings(ctx) }" to call s.GetAutomationSettings(ctx) and handle its error explicitly: if an error is returned, log it (using the existing logger in this scope) and set settings = models.DefaultCrossSeedAutomationSettings(); if no error but settings is still nil, also set the default. Ensure you reference the GetAutomationSettings method and models.DefaultCrossSeedAutomationSettings() when making these changes so callers never see a nil settings value.
8917-8929:⚠️ Potential issue | 🟠 MajorThe normalized specific-domain fallback is still label-insensitive.
Lines 8927-8929 still accept substring matches after TLD stripping, so unrelated hosts like
notredactedandredactedcan be treated as the same indexer domain. That can exclude the wrong indexer or attribute content to the wrong tracker. Let the portable matcher cover the looser cases and keep this branch to exact normalized domain-name matches.💡 Possible fix
- if domainWithoutTLD == normalizedIndexerDomainName || - (len(normalizedIndexerDomainName) >= 6 && strings.Contains(domainWithoutTLD, normalizedIndexerDomainName)) || - (len(domainWithoutTLD) >= 6 && strings.Contains(normalizedIndexerDomainName, domainWithoutTLD)) { + if domainWithoutTLD == normalizedIndexerDomainName {🤖 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 8917 - 8929, The fallback branch for normalized specific-domain matching currently allows substring matches and should be restricted to exact, label-aware equality: in the block using indexerDomainWithoutTLD, domainWithoutTLD, normalizedIndexerDomainName and the call s.normalizeDomainName(strip/normalize functions), remove the two len/strings.Contains conditions and only keep the equality check (domainWithoutTLD == normalizedIndexerDomainName) so the branch only accepts exact normalized domain-name matches after TLD stripping; leave the looser portable matcher logic untouched.
🧹 Nitpick comments (1)
web/src/pages/CrossSeedPage.tsx (1)
335-574: Extract the tracker-mapping editor into its own feature component.This block now carries its own queries, mutations, and local UI state on top of an already very large page module. Moving
InstanceTrackerCategoryMappings— and optionallyCategoryComboboxbeside it — intoweb/src/components/cross-seed/would make the page easier to test and keep page-level logic focused on orchestration.As per coding guidelines
web/src/{pages,routes,components}/**/*.{ts,tsx}: Organize React modules by feature within web/src/{pages,routes,components} with descriptive file names (e.g., torrent-table.tsx).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/CrossSeedPage.tsx` around lines 335 - 574, Split the InstanceTrackerCategoryMappings (and optionally CategoryCombobox) out of the large page into a new feature component file under web/src/components/cross-seed/ (e.g., instance-tracker-category-mappings.tsx); move the entire function bodies (including their local state, useQuery/useMutation calls, helper useMemo logic, and related handlers like resetAddingRow) into that file, export the main component (default or named), and export CategoryCombobox if kept separate; update the original CrossSeedPage to import the new component(s) and remove the duplicated definitions so all queries, mutations, and UI state live inside the extracted component file.
🤖 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/crossseed.go`:
- Around line 1717-1738: The handler currently calls h.indexerCategoryStore.Set
and surface FK failures as 500; first verify the indexer exists and is not
deleted before calling Set (e.g., call the appropriate lookup method such as
h.indexerStore.Get or h.indexerStore.Exists with instanceID and req.IndexerID)
and return a 404/400 if it does not exist, or catch the specific
foreign-key/constraint error returned by h.indexerCategoryStore.Set and
translate that to a 404/400 with a clear client-facing message instead of
returning 500; ensure references to instanceID and req.IndexerID are included in
logs and error responses for diagnostics.
In `@internal/proxy/handler.go`:
- Around line 2135-2148: The failure branches in the handler currently call
http.Error and return text/plain, causing inconsistent response formats; replace
those http.Error calls (the ones checking instanceID == 0,
h.indexerCategoryStore == nil, and the error branch after
h.indexerCategoryStore.List(ctx, instanceID)) with the existing JSON helper
writeJSONError so failures return application/json. Specifically, use
writeJSONError(w, statusCode, message) (or the local helper signature used at
Line 622) instead of http.Error, and keep the same log.Error(...) call for the
List error path (log.Error().Err(err).Int("instanceId", instanceID).Msg(...))
while invoking writeJSONError with an appropriate message and http status to
preserve the wire format for proxy clients.
In `@internal/services/dirscan/inject.go`:
- Around line 525-541: The branch that handles req.TrackerCategorySavePath
currently sets destDir = selectedBaseDir which makes the real category directory
the plan/rollback root; change this so plan.RootDir is a newly created
subdirectory (or a dedicated staging directory) inside selectedBaseDir instead
of selectedBaseDir itself. In other words, after ensuring selectedBaseDir
exists, create a unique staging directory under selectedBaseDir (or use your
existing staging helper) and set destDir (and thus the plan.RootDir used by
rollbackLinkTree) to that new subdirectory so rollbackLinkTree will never remove
the persistent category directory; keep references to selectedBaseDir, destDir,
req.TrackerCategorySavePath, rollbackLinkTree and plan.RootDir to locate where
to change behavior.
In `@internal/services/dirscan/service.go`:
- Around line 1791-1815: The resolved tracker category (trackerCat) should be
retained even if the qBittorrent save-path lookup fails; modify the block after
crossseed.ResolveTrackerCategory returns found=true so that you assign category
= trackerCat immediately, then call s.syncManager.GetCategories to try to
populate trackerCategorySavePath, but if GetCategories returns an error only log
the failure and continue (do not revert category to the global path or return).
In other words, keep the trackerCat assignment before calling
s.syncManager.GetCategories and only conditionally set trackerCategorySavePath
from qbitCats when catErr == nil; leave trackerCategorySavePath empty on catErr
and proceed.
---
Duplicate comments:
In `@internal/services/crossseed/service.go`:
- Around line 10041-10043: When settings == nil after calling
s.GetAutomationSettings(ctx) we must not silently ignore the error; update the
block that currently does "if settings == nil { settings, _ =
s.GetAutomationSettings(ctx) }" to call s.GetAutomationSettings(ctx) and handle
its error explicitly: if an error is returned, log it (using the existing logger
in this scope) and set settings = models.DefaultCrossSeedAutomationSettings();
if no error but settings is still nil, also set the default. Ensure you
reference the GetAutomationSettings method and
models.DefaultCrossSeedAutomationSettings() when making these changes so callers
never see a nil settings value.
- Around line 8917-8929: The fallback branch for normalized specific-domain
matching currently allows substring matches and should be restricted to exact,
label-aware equality: in the block using indexerDomainWithoutTLD,
domainWithoutTLD, normalizedIndexerDomainName and the call
s.normalizeDomainName(strip/normalize functions), remove the two
len/strings.Contains conditions and only keep the equality check
(domainWithoutTLD == normalizedIndexerDomainName) so the branch only accepts
exact normalized domain-name matches after TLD stripping; leave the looser
portable matcher logic untouched.
---
Nitpick comments:
In `@web/src/pages/CrossSeedPage.tsx`:
- Around line 335-574: Split the InstanceTrackerCategoryMappings (and optionally
CategoryCombobox) out of the large page into a new feature component file under
web/src/components/cross-seed/ (e.g., instance-tracker-category-mappings.tsx);
move the entire function bodies (including their local state,
useQuery/useMutation calls, helper useMemo logic, and related handlers like
resetAddingRow) into that file, export the main component (default or named),
and export CategoryCombobox if kept separate; update the original CrossSeedPage
to import the new component(s) and remove the duplicated definitions so all
queries, mutations, and UI state live inside the extracted component file.
🪄 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: dce1bf5f-8904-4922-b35d-cbfa8e6785b7
📒 Files selected for processing (28)
cmd/qui/main.gointernal/api/handlers/crossseed.gointernal/api/handlers/dirscan_webhook_test.gointernal/api/server.gointernal/api/server_test.gointernal/database/migrations/069_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/crossseed_test.gointernal/services/crossseed/domain_aliases.gointernal/services/crossseed/domain_aliases_test.gointernal/services/crossseed/hardlink_mode_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/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 (9)
- cmd/qui/main.go
- internal/services/dirscan/inject_test.go
- internal/database/migrations/069_add_tracker_category_settings.sql
- internal/services/crossseed/domain_aliases.go
- web/src/lib/api.ts
- internal/models/crossseed_indexer_categories_test.go
- internal/services/crossseed/tracker_category_test.go
- internal/proxy/crossseed_indexer_categories_test.go
- internal/web/swagger/openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/api/server_test.go
- internal/services/dirscan/cancel_scan_test.go
- web/src/types/index.ts
- internal/proxy/handler_test.go
- internal/services/crossseed/domain_aliases_test.go
- internal/api/server.go
- internal/services/crossseed/hardlink_mode_test.go
- internal/api/handlers/dirscan_webhook_test.go
- internal/services/crossseed/crossseed_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
internal/services/dirscan/inject.go (1)
527-543: Duplicate inverse path mapping computation.The inverse mapping of
TrackerCategorySavePathis computed twice:
- Lines 497-504: Stored in
effectiveBaseDir- Lines 532-538: Stored in
selectedBaseDirBoth use identical parameters (
req.QbitPathPrefix,filepath.Dir(req.Searchee.Path)). Consider reusingeffectiveBaseDirto avoid duplicate computation and potential divergence.♻️ Suggested simplification
if req.TrackerCategorySavePath != "" { // Tracker-category mode: files must land directly inside the category save // path — no isolation subfolder — so AutoTMM can manage placement. - // TrackerCategorySavePath is qBittorrent's view of the path; translate it - // to a host filesystem path when QbitPathPrefix is configured. - selectedBaseDir = req.TrackerCategorySavePath - if req.QbitPathPrefix != "" && req.Searchee != nil { - mapped, ok := applyInversePathMapping(selectedBaseDir, filepath.Dir(req.Searchee.Path), req.QbitPathPrefix) - if !ok { - return nil, "", fmt.Errorf("tracker category save path %q cannot be mapped to host filesystem: QbitPathPrefix %q does not match", selectedBaseDir, req.QbitPathPrefix) - } - selectedBaseDir = mapped - } + // effectiveBaseDir was already computed and mapped above. + selectedBaseDir = effectiveBaseDir if err := os.MkdirAll(selectedBaseDir, 0o755); err != nil { return nil, "", fmt.Errorf("create tracker category dir %q: %w", selectedBaseDir, err) }🤖 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 527 - 543, The code recomputes the inverse mapping for TrackerCategorySavePath in two places; replace the second applyInversePathMapping call in the TrackerCategorySavePath branch by reusing the previously computed effectiveBaseDir: when req.TrackerCategorySavePath != "" and req.QbitPathPrefix != "" && req.Searchee != nil, use effectiveBaseDir (the mapped host path computed earlier with applyInversePathMapping) as selectedBaseDir instead of calling applyInversePathMapping again; ensure error handling and the os.MkdirAll call continue to reference selectedBaseDir (now set from effectiveBaseDir) so behavior is unchanged but duplicate computation is removed.web/src/pages/CrossSeedPage.tsx (1)
466-468: Call out the unmapped-indexer fallback here.This reads as if every enabled indexer needs a row, but the preset falls back to the global resolution order (
custom → indexer → affix → reuse) when no mapping exists. Adding that sentence here would make the setup much clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/CrossSeedPage.tsx` around lines 466 - 468, In the CrossSeedPage component, update the explanatory paragraph (the <p className="text-xs text-muted-foreground"> block) to explicitly mention the unmapped-indexer fallback: note that if an enabled indexer has no explicit category mapping, the preset resolves using the global fallback order "custom → indexer → affix → reuse". Add a single sentence after the existing text clarifying this fallback behavior so users understand that rows aren’t required for every enabled indexer.
🤖 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/crossseed.go`:
- Around line 1734-1738: The current handler collapses any FK failure from
h.indexerCategoryStore.Set into "indexer not found"; instead either detect the
specific FK constraint or explicitly validate the indexer before writing: call
the existing indexer lookup (e.g., h.indexerStore.Exists or Get with instanceID
and req.IndexerID) prior to invoking h.indexerCategoryStore.Set and return
RespondError(w, http.StatusBadRequest, "indexer not found") only when that
lookup fails; for other FK errors (or if you prefer DB inspection) match the
constraint name/error code from the returned error string from Set to
distinguish an instance_id FK vs indexer_id FK and return an appropriate error
(e.g., stale instance) rather than always "indexer not found".
- Around line 1729-1734: Validation trims whitespace but persists the original
req.Category, so update the code to persist the trimmed value instead: compute
the trimmed string (using strings.TrimSpace on req.Category) and pass that
trimmed value into indexerCategoryStore.Set (and/or assign it back to
req.Category) so the stored category matches the validated value; reference
indexerCategoryStore.Set and req.Category in your change.
In `@internal/services/crossseed/service.go`:
- Around line 4423-4424: The code currently calls
determineCrossSeedCategory(...) after ParseTorrentAnnounceDomain(...) but
ignores errors from s.instanceStore.Get(ctx, candidate.InstanceID) so
instance.HardlinkDirPreset may be unavailable and tracker-mode adds get silently
downgraded; change the flow to check and handle the error returned by
s.instanceStore.Get (and fail/return the error) before calling
determineCrossSeedCategory, ensuring determineCrossSeedCategory is only invoked
with a valid instance (so instance.HardlinkDirPreset is defined) and propagate
the lookup error upstream instead of continuing on transient store/DB failures.
- Around line 11121-11128: The code conflates the intended add target path with
the persisted category save_path by reusing effectiveCategorySavePath (computed
via s.buildCategorySavePath) as both the "add" target and the "category exists"
indicator; as a result ensureCrossCategory(...) is called with "" for fresh
hardlink categories and AutoTMM later treats the synthesized path as evidence
qBittorrent already configured the category. Fix by splitting concerns:
introduce a distinct variable (e.g., desiredAddPath or addTargetPath) computed
from s.buildCategorySavePath(instance, selectedBaseDir, incomingTrackerDomain,
candidate, req) when mapping applies, keep the original categorySavePath
(persistedCategorySavePath) as the persisted/configured qB path, and pass
addTargetPath to ensureCrossCategory(...) while leaving
persistedCategorySavePath untouched for existence checks and AutoTMM logic;
apply the same separation in the other similar blocks (the occurrences
referenced around ensureCrossCategory, the AutoTMM branch, and the other ranges
noted).
---
Nitpick comments:
In `@internal/services/dirscan/inject.go`:
- Around line 527-543: The code recomputes the inverse mapping for
TrackerCategorySavePath in two places; replace the second
applyInversePathMapping call in the TrackerCategorySavePath branch by reusing
the previously computed effectiveBaseDir: when req.TrackerCategorySavePath != ""
and req.QbitPathPrefix != "" && req.Searchee != nil, use effectiveBaseDir (the
mapped host path computed earlier with applyInversePathMapping) as
selectedBaseDir instead of calling applyInversePathMapping again; ensure error
handling and the os.MkdirAll call continue to reference selectedBaseDir (now set
from effectiveBaseDir) so behavior is unchanged but duplicate computation is
removed.
In `@web/src/pages/CrossSeedPage.tsx`:
- Around line 466-468: In the CrossSeedPage component, update the explanatory
paragraph (the <p className="text-xs text-muted-foreground"> block) to
explicitly mention the unmapped-indexer fallback: note that if an enabled
indexer has no explicit category mapping, the preset resolves using the global
fallback order "custom → indexer → affix → reuse". Add a single sentence after
the existing text clarifying this fallback behavior so users understand that
rows aren’t required for every enabled indexer.
🪄 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: aff0ca1f-04c9-4791-9ba2-dfc434a2db1b
📒 Files selected for processing (28)
cmd/qui/main.gointernal/api/handlers/crossseed.gointernal/api/handlers/dirscan_webhook_test.gointernal/api/server.gointernal/api/server_test.gointernal/database/migrations/069_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/crossseed_test.gointernal/services/crossseed/domain_aliases.gointernal/services/crossseed/domain_aliases_test.gointernal/services/crossseed/hardlink_mode_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/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 (12)
- internal/api/server_test.go
- internal/api/handlers/dirscan_webhook_test.go
- web/src/types/index.ts
- internal/database/migrations/069_add_tracker_category_settings.sql
- internal/services/crossseed/domain_aliases.go
- internal/models/crossseed_indexer_categories_test.go
- internal/services/crossseed/auto_tmm_test.go
- internal/services/dirscan/inject_test.go
- internal/proxy/crossseed_indexer_categories_test.go
- internal/services/crossseed/tracker_category_test.go
- internal/services/dirscan/service.go
- internal/web/swagger/openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/services/dirscan/webhook_queue_test.go
- internal/proxy/handler_test.go
- internal/api/server.go
- internal/services/dirscan/cancel_scan_test.go
- internal/services/crossseed/hardlink_mode_test.go
- cmd/qui/main.go
- internal/services/crossseed/crossseed_test.go
CjE5
force-pushed
the
develop
branch
2 times, most recently
from
April 5, 2026 19:10
01822a2 to
a5a83b3
Compare
Contributor
Author
|
@coderabbitai resume |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
internal/services/crossseed/service.go (2)
4253-4257:⚠️ Potential issue | 🟠 MajorFail closed when the target instance lookup fails.
This warn-and-continue path drops
HardlinkDirPreset, hardlink/reflink mode, and tracker-category resolution from the decision tree, so a transientinstanceStore.Get(...)failure can inject into the wrong category/save path for a trustedcandidate.InstanceID. Return an error result here instead of silently degrading to a standard add.As per coding guidelines: “Prefer explicit error handling over silent failures in Go.”💡 Possible fix
instance, instanceErr := s.instanceStore.Get(ctx, candidate.InstanceID) if instanceErr != nil { - log.Warn().Err(instanceErr).Int("instanceID", candidate.InstanceID). - Msg("[CROSSSEED] Failed to load instance settings; hardlink/tracker-category mode unavailable, falling back to standard add") + log.Warn().Err(instanceErr).Int("instanceID", candidate.InstanceID). + Msg("[CROSSSEED] Failed to load instance settings") + result.Message = fmt.Sprintf("Failed to load instance settings: %v", instanceErr) + return result +} +if instance == nil { + result.Message = fmt.Sprintf("Target instance %d not found", candidate.InstanceID) + return result } useReflinkMode := instanceErr == nil && instance != nil && instance.UseReflinks useHardlinkMode := instanceErr == nil && instance != nil && instance.UseHardlinks && !instance.UseReflinks🤖 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 4253 - 4257, The current warn-and-continue when s.instanceStore.Get(ctx, candidate.InstanceID) fails silently drops HardlinkDirPreset, hardlink/reflink mode, and tracker-category resolution; change this to fail closed by returning the error up the call path instead of falling back to a standard add. Specifically, in the block that calls s.instanceStore.Get(...) for candidate.InstanceID, replace the log-and-continue behavior by returning (or wrapping) instanceErr from the surrounding function (propagate the error to the caller) so the add flow aborts and the caller can retry or handle the lookup failure; keep the existing log but ensure the function returns the error to prevent incorrect categorization based on missing instance settings.
4445-4453:⚠️ Potential issue | 🟠 MajorKeep the actual qB category path separate from the derived tracker add path.
actualCategorySavePathis populated from the base category template here, then the hardlink flow later treats that value as if qB had already configured the cross-category there. That makes the new save-path override apply outside tracker-mapped adds, and it can still flipautoTMM=truefromeffectiveCategorySavePathwhen an existing mapped category has no explicit qBsave_path. Please split these concerns: actual configured cross-category path, desired tracker add path, and temporary hardlink root. Only derive/use the tracker path whenisTrackerCategoryModeis true, and only enableautoTMMfrom the cross-category’s actual configured qB path.Based on learnings: “Repo: autobrr/qui — Cross-seed ‘by-tracker’ preset: Do not suggest synthesizing category save paths or AutoTMM behavior for ‘by-instance’ or ‘flat’ presets; tracker-mapping-driven behavior is intentionally limited to by-tracker only in PR
#1722.”Also applies to: 11071-11098, 11130-11155, 11265-11270
🤖 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 4445 - 4453, actualCategorySavePath is being populated from baseCategory template and later misused as if qBittorrent already configured that path; split concerns by keeping three distinct values: actualCategorySavePath (only set when categories[crossCategory] exists with a real qB SavePath), trackerAddPath (derived from baseCategory template or tracker mapping but only when isTrackerCategoryMode is true), and hardlinkRoot (temporary path used in the hardlink flow). Update the logic around categories, crossCategory, baseCategory, categorySavePath, effectiveCategorySavePath and isTrackerCategoryMode so that: 1) do not assign actualCategorySavePath from baseCategory templates, 2) only compute and use trackerAddPath when isTrackerCategoryMode is true, and 3) enable autoTMM only when actualCategorySavePath came from an existing qB category (categories[crossCategory].SavePath), ensuring the hardlink flow uses hardlinkRoot/trackerAddPath appropriately instead of conflating with actualCategorySavePath.
🧹 Nitpick comments (1)
internal/models/crossseed_indexer_categories_test.go (1)
114-128: Add regression cases for blank categories and trimmed lookups.These tests cover successful trimming, but they still miss the two validation edges that were just fixed in the store:
" "should be rejected bySet, and" Aither "should resolve the same as"Aither"inGetByIndexerName. A small table-driven case here would keep both from regressing.As per coding guidelines, "Prefer table-driven test cases in Go tests".
Also applies to: 160-214
🤖 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 114 - 128, Add table-driven test cases to cover two regressions: ensure store.Set rejects purely-blank categories (e.g., " ") and ensure lookups via GetByIndexerName treat trimmed names equivalently (e.g., " Aither " resolves the same as "Aither"). Update the existing TestCrossSeedIndexerCategoryStore_SetTrimsCategoryWhitespace (and related tests around the 160-214 region) to iterate over cases that call store.Set and assert an error for blank-category inputs, and to insert a trimmed category then call store.GetByIndexerName with both trimmed and untrimmed indexerName values and assert the same mapping is returned; reference the store methods Set and GetByIndexerName and the test helper insertTestInstance/insertTestTorznabIndexer to locate where to add the table-driven cases.
🤖 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/inject.go`:
- Around line 455-464: The by-tracker fast path currently uses
TrackerCategorySavePath even when needsIsolation is true, which breaks
per-release isolation; modify the branch that sets options["autoTMM"] and
options["savepath"] so that if needsIsolation is true you do not use the shared
TrackerCategorySavePath (i.e., gate the by-tracker fast path on !needsIsolation)
or explicitly clear TrackerCategorySavePath before building options, ensuring
that when needsIsolation is true the code falls through to the standard-mode
branch that sets options["savepath"]=savePath and disables autoTMM; update logic
around needsIsolation, TrackerCategorySavePath, and the options map to preserve
isolated destDir for single-file/rootless torrents.
In `@internal/services/dirscan/service.go`:
- Around line 1785-1818: The by-tracker branch drops out when
ParseTorrentAnnounceDomain returns empty or the domain doesn't resolve; mirror
the crossseed fallback by attempting a secondary ResolveTrackerCategory using
the torrent's indexer metadata (e.g. result.Indexer or result.IndexerID) before
giving up on the by-tracker preset. Update the block around
ParseTorrentAnnounceDomain / ResolveTrackerCategory (and the related variables
trackerCategorySavePath, s.indexerCategoryStore, s.instanceStore, and the
HardlinkDirPreset "by-tracker" check) so that if announceDomain=="" or
ResolveTrackerCategory returns not found you call ResolveTrackerCategory again
with the indexer identifier, apply the same category assignment and qBittorrent
save-path lookup (s.syncManager.GetCategories) when that resolves, and log the
fallback attempt/results appropriately.
---
Duplicate comments:
In `@internal/services/crossseed/service.go`:
- Around line 4253-4257: The current warn-and-continue when
s.instanceStore.Get(ctx, candidate.InstanceID) fails silently drops
HardlinkDirPreset, hardlink/reflink mode, and tracker-category resolution;
change this to fail closed by returning the error up the call path instead of
falling back to a standard add. Specifically, in the block that calls
s.instanceStore.Get(...) for candidate.InstanceID, replace the log-and-continue
behavior by returning (or wrapping) instanceErr from the surrounding function
(propagate the error to the caller) so the add flow aborts and the caller can
retry or handle the lookup failure; keep the existing log but ensure the
function returns the error to prevent incorrect categorization based on missing
instance settings.
- Around line 4445-4453: actualCategorySavePath is being populated from
baseCategory template and later misused as if qBittorrent already configured
that path; split concerns by keeping three distinct values:
actualCategorySavePath (only set when categories[crossCategory] exists with a
real qB SavePath), trackerAddPath (derived from baseCategory template or tracker
mapping but only when isTrackerCategoryMode is true), and hardlinkRoot
(temporary path used in the hardlink flow). Update the logic around categories,
crossCategory, baseCategory, categorySavePath, effectiveCategorySavePath and
isTrackerCategoryMode so that: 1) do not assign actualCategorySavePath from
baseCategory templates, 2) only compute and use trackerAddPath when
isTrackerCategoryMode is true, and 3) enable autoTMM only when
actualCategorySavePath came from an existing qB category
(categories[crossCategory].SavePath), ensuring the hardlink flow uses
hardlinkRoot/trackerAddPath appropriately instead of conflating with
actualCategorySavePath.
---
Nitpick comments:
In `@internal/models/crossseed_indexer_categories_test.go`:
- Around line 114-128: Add table-driven test cases to cover two regressions:
ensure store.Set rejects purely-blank categories (e.g., " ") and ensure
lookups via GetByIndexerName treat trimmed names equivalently (e.g., " Aither
" resolves the same as "Aither"). Update the existing
TestCrossSeedIndexerCategoryStore_SetTrimsCategoryWhitespace (and related tests
around the 160-214 region) to iterate over cases that call store.Set and assert
an error for blank-category inputs, and to insert a trimmed category then call
store.GetByIndexerName with both trimmed and untrimmed indexerName values and
assert the same mapping is returned; reference the store methods Set and
GetByIndexerName and the test helper insertTestInstance/insertTestTorznabIndexer
to locate where to add the table-driven cases.
🪄 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: 5e6873c4-ef59-4f61-aa97-2b2b7dcd7fdc
📒 Files selected for processing (28)
cmd/qui/main.gointernal/api/handlers/crossseed.gointernal/api/handlers/dirscan_webhook_test.gointernal/api/server.gointernal/api/server_test.gointernal/database/migrations/069_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/crossseed_test.gointernal/services/crossseed/domain_aliases.gointernal/services/crossseed/domain_aliases_test.gointernal/services/crossseed/hardlink_mode_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/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 (8)
- internal/api/handlers/dirscan_webhook_test.go
- internal/services/crossseed/domain_aliases.go
- internal/services/crossseed/crossseed_test.go
- internal/database/migrations/069_add_tracker_category_settings.sql
- internal/services/crossseed/tracker_category_test.go
- internal/proxy/crossseed_indexer_categories_test.go
- web/src/pages/CrossSeedPage.tsx
- internal/web/swagger/openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/api/server_test.go
- internal/services/dirscan/cancel_scan_test.go
- cmd/qui/main.go
- web/src/types/index.ts
- internal/services/crossseed/hardlink_mode_test.go
- internal/api/server.go
- internal/proxy/handler_test.go
- internal/services/crossseed/tracker_category.go
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
internal/services/crossseed/service.go (2)
4445-4454:⚠️ Potential issue | 🟠 MajorDon't treat template tracker paths as configured qB category paths.
actualCategorySavePathfalls back tobaseCategoryhere, and hardlink mode later treatseffectiveCategorySavePathas proof that the mapped category is configured. That is unsafe whencrossCategoryalready exists without its ownsave_pathor when the real qB category path is otherwise unknown:ensureCrossCategory(...)won't repair that existing category, so tracker-modeautoTMM=truecan still send qB to a different directory than the prelinked tree. Keep the desired/template path separate from the mapped category’s actual configured save path, and gate tracker-mode AutoTMM on the actual value only.Based on learnings: “ensureCrossCategory intentionally keeps warn-only behavior when an existing category’s save_path differs from the desired path and still caches it in createdCategories; callers should not treat this as categoryCreationFailed nor attempt repair.”
Also applies to: 4471-4471, 11130-11132, 11265-11269
🤖 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 4445 - 4454, The code incorrectly treats the template/base category path as the actual configured qBittorrent category path by assigning actualCategorySavePath = cat.SavePath for baseCategory when crossCategory exists without its own SavePath; change the logic in the block that sets categorySavePath and actualCategorySavePath so that actualCategorySavePath is only set from the existing mapped category (crossCategory) when that category has a non-empty SavePath, while categorySavePath (the template/desired path) may still be taken from baseCategory; ensure callers and later logic (e.g., hardlink mode using effectiveCategorySavePath and any AutoTMM gating) only consider the actualCategorySavePath when deciding whether the mapped category is configured or whether to attempt repairs, and preserve ensureCrossCategory’s warn-only behavior (and createdCategories caching) instead of treating template-derived values as proof of configuration.
4253-4257:⚠️ Potential issue | 🟠 MajorFail fast when the instance lookup fails.
If
instanceStore.Geterrors here, the rest of the method continues with a nil/unknowninstance, which silently disables by-tracker resolution and link-mode handling for this candidate. That can inject into the wrong category/save path on transient store failures; this path should return an error result instead of falling back.💡 Possible fix
instance, instanceErr := s.instanceStore.Get(ctx, candidate.InstanceID) - if instanceErr != nil { - log.Warn().Err(instanceErr).Int("instanceID", candidate.InstanceID). - Msg("[CROSSSEED] Failed to load instance settings; hardlink/tracker-category mode unavailable, falling back to standard add") - } + if instanceErr != nil { + log.Warn().Err(instanceErr).Int("instanceID", candidate.InstanceID). + Msg("[CROSSSEED] Failed to load instance settings; aborting cross-seed candidate") + result.Message = fmt.Sprintf("Failed to load instance settings: %v", instanceErr) + return result + } + if instance == nil { + log.Warn().Int("instanceID", candidate.InstanceID). + Msg("[CROSSSEED] Instance settings missing; aborting cross-seed candidate") + result.Message = fmt.Sprintf("Instance %d not found", candidate.InstanceID) + return result + }As per coding guidelines: “Prefer explicit error handling over silent failures in Go.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/crossseed/service.go` around lines 4253 - 4257, The instance lookup result from s.instanceStore.Get used for candidate.InstanceID must not be silently ignored; in the function containing this call, check instanceErr and return it (or wrap and return a descriptive error) immediately instead of merely logging and continuing with a nil instance, so that by-tracker resolution and link-mode handling are not silently disabled; update the block around s.instanceStore.Get(...) that assigns instance, instanceErr (and any surrounding function return paths) to propagate the error up (e.g., return fmt.Errorf or the existing error type) when instanceErr != nil, referencing the variables instance, instanceErr, and candidate.InstanceID to locate and fix the call site.internal/services/dirscan/inject.go (1)
517-517:⚠️ Potential issue | 🟠 MajorKeep isolation for rootless tracker-category adds.
Line 517 computes
needsIsolation, but this branch still writes straight into the shared tracker-category directory. For single-file/rootless torrents, later injections from the same tracker can collide on top-level names, andautoTMM + contentLayout=Originalwill not recreate a per-release folder. Gate the fast path on!needsIsolation, or clearTrackerCategorySavePathso these cases fall back to the isolated save-path flow.Also applies to: 527-536
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/dirscan/inject.go` at line 517, The code computes needsIsolation using needsIsolation := !hardlinktree.HasCommonRootFolder(incomingFiles) but still takes the fast-path that writes directly into the shared tracker-category directory; change the branching so the fast-path is only used when needsIsolation is false (i.e., gate the fast path on !needsIsolation), or explicitly clear/override TrackerCategorySavePath when needsIsolation is true so the logic falls back to the isolated save-path flow; update the same pattern around the related block (the section referenced at lines ~527-536) to use the same gate/clear behavior to avoid top-level name collisions for rootless/single-file tracker-category adds.
🧹 Nitpick comments (2)
internal/api/handlers/crossseed.go (1)
1668-1679: Extract the repeated instance validation into one helper.The same
instanceStore.Get→404/500branch now appears in all three new handlers, and the completion endpoints above already carry a near-identical copy. Centralizing it will keep the instance-scoped handlers from drifting on response messages or logging.Also applies to: 1704-1715, 1768-1779
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handlers/crossseed.go` around lines 1668 - 1679, Extract the repeated instance validation logic into a single helper on the handler (e.g., add a method like func (h *Handler) validateInstance(ctx context.Context, instanceID int, w http.ResponseWriter) error) that calls h.instanceStore.Get, checks errors.Is(err, models.ErrInstanceNotFound) to RespondError(w, http.StatusNotFound, "Instance not found"), logs other errors with log.Error().Err(err).Int("instanceID", instanceID).Msg("Failed to validate instance for indexer categories") and RespondError(w, http.StatusInternalServerError, "Failed to validate instance") before returning the error; then replace the duplicated blocks (the occurrences around instanceStore.Get in the handlers and at the other noted locations) with a single call to that helper and early return on non-nil error.internal/proxy/crossseed_indexer_categories_test.go (1)
81-106: Assert the JSON error contract in the failure cases too.These two tests only check status codes, so a regression back to
http.Errorwould still pass. Please also assertContent-Type: application/jsonand the{"error":...}payload for the missing-instance and nil-store branches.🤖 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 - 106, Tests TestHandleCrossSeedIndexerCategories_MissingInstanceID and TestHandleCrossSeedIndexerCategories_NilStore only assert status codes; update them to also assert the JSON error contract by checking that the response header "Content-Type" equals "application/json" and that the body unmarshals to a map[string]string (or struct) containing an "error" key with the expected message. Locate the tests and add assertions after h.handleCrossSeedIndexerCategories(rec, req): check rec.Header().Get("Content-Type") and decode rec.Body into a small variable then assert the "error" field is non-empty (or matches the expected text for each branch) while still asserting the status codes; reference TestHandleCrossSeedIndexerCategories_MissingInstanceID, TestHandleCrossSeedIndexerCategories_NilStore, handleCrossSeedIndexerCategories, and InstanceIDContextKey to find where to add these checks.
🤖 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/tracker_category.go`:
- Around line 128-153: ResolveTrackerCategory must prefer exact indexer identity
over announce-domain heuristics; change its signature to accept the caller's
indexer identity (e.g., add indexerID int and/or indexerName string parameters)
and, after loading mappings from models.CrossSeedIndexerCategoryStore, first
search mappings for an exact match (prefer m.IndexerID == indexerID if provided,
otherwise m.IndexerName == indexerName) and return that category if found; only
if no exact match exists fall back to the existing loop that uses
MatchAnnounceDomainToIndexer(announceDomain, m.IndexerName). Update callers to
pass the indexer identity where available so duplicate tracker entries resolve
deterministically.
- Around line 138-149: The mapping lookup in ResolveTrackerCategory currently
swallows store.List errors and returns ("", false, nil), causing a silent
fallback; change the error handling so that if store.List returns an error and
ctx.Err() is nil you log the error (as done) and then propagate that error to
the caller (return "", false, err) instead of nil. Locate the block around the
call to store.List(ctx, instanceID) and replace the final return value to return
the original err so callers can fail closed.
In `@web/src/pages/CrossSeedPage.tsx`:
- Around line 466-468: The help text in CrossSeedPage.tsx currently uses
mode-specific wording ("hardlinked" / "hardlinks are created") in the paragraph
with className "text-xs text-muted-foreground", which is inaccurate for reflink
mode; update the copy to be mode-neutral (e.g., "linked using the configured
link mode (hardlink or reflink)" and "links are created" or "files are
transferred using the configured link mode") and change both occurrences in that
paragraph so the description correctly applies to both hardlink and reflink
modes.
---
Duplicate comments:
In `@internal/services/crossseed/service.go`:
- Around line 4445-4454: The code incorrectly treats the template/base category
path as the actual configured qBittorrent category path by assigning
actualCategorySavePath = cat.SavePath for baseCategory when crossCategory exists
without its own SavePath; change the logic in the block that sets
categorySavePath and actualCategorySavePath so that actualCategorySavePath is
only set from the existing mapped category (crossCategory) when that category
has a non-empty SavePath, while categorySavePath (the template/desired path) may
still be taken from baseCategory; ensure callers and later logic (e.g., hardlink
mode using effectiveCategorySavePath and any AutoTMM gating) only consider the
actualCategorySavePath when deciding whether the mapped category is configured
or whether to attempt repairs, and preserve ensureCrossCategory’s warn-only
behavior (and createdCategories caching) instead of treating template-derived
values as proof of configuration.
- Around line 4253-4257: The instance lookup result from s.instanceStore.Get
used for candidate.InstanceID must not be silently ignored; in the function
containing this call, check instanceErr and return it (or wrap and return a
descriptive error) immediately instead of merely logging and continuing with a
nil instance, so that by-tracker resolution and link-mode handling are not
silently disabled; update the block around s.instanceStore.Get(...) that assigns
instance, instanceErr (and any surrounding function return paths) to propagate
the error up (e.g., return fmt.Errorf or the existing error type) when
instanceErr != nil, referencing the variables instance, instanceErr, and
candidate.InstanceID to locate and fix the call site.
In `@internal/services/dirscan/inject.go`:
- Line 517: The code computes needsIsolation using needsIsolation :=
!hardlinktree.HasCommonRootFolder(incomingFiles) but still takes the fast-path
that writes directly into the shared tracker-category directory; change the
branching so the fast-path is only used when needsIsolation is false (i.e., gate
the fast path on !needsIsolation), or explicitly clear/override
TrackerCategorySavePath when needsIsolation is true so the logic falls back to
the isolated save-path flow; update the same pattern around the related block
(the section referenced at lines ~527-536) to use the same gate/clear behavior
to avoid top-level name collisions for rootless/single-file tracker-category
adds.
---
Nitpick comments:
In `@internal/api/handlers/crossseed.go`:
- Around line 1668-1679: Extract the repeated instance validation logic into a
single helper on the handler (e.g., add a method like func (h *Handler)
validateInstance(ctx context.Context, instanceID int, w http.ResponseWriter)
error) that calls h.instanceStore.Get, checks errors.Is(err,
models.ErrInstanceNotFound) to RespondError(w, http.StatusNotFound, "Instance
not found"), logs other errors with log.Error().Err(err).Int("instanceID",
instanceID).Msg("Failed to validate instance for indexer categories") and
RespondError(w, http.StatusInternalServerError, "Failed to validate instance")
before returning the error; then replace the duplicated blocks (the occurrences
around instanceStore.Get in the handlers and at the other noted locations) with
a single call to that helper and early return on non-nil error.
In `@internal/proxy/crossseed_indexer_categories_test.go`:
- Around line 81-106: Tests
TestHandleCrossSeedIndexerCategories_MissingInstanceID and
TestHandleCrossSeedIndexerCategories_NilStore only assert status codes; update
them to also assert the JSON error contract by checking that the response header
"Content-Type" equals "application/json" and that the body unmarshals to a
map[string]string (or struct) containing an "error" key with the expected
message. Locate the tests and add assertions after
h.handleCrossSeedIndexerCategories(rec, req): check
rec.Header().Get("Content-Type") and decode rec.Body into a small variable then
assert the "error" field is non-empty (or matches the expected text for each
branch) while still asserting the status codes; reference
TestHandleCrossSeedIndexerCategories_MissingInstanceID,
TestHandleCrossSeedIndexerCategories_NilStore, handleCrossSeedIndexerCategories,
and InstanceIDContextKey to find where to add these checks.
🪄 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: 529c5070-198a-43c7-b0cf-8a5970dd334a
📒 Files selected for processing (28)
cmd/qui/main.gointernal/api/handlers/crossseed.gointernal/api/handlers/dirscan_webhook_test.gointernal/api/server.gointernal/api/server_test.gointernal/database/migrations/069_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/crossseed_test.gointernal/services/crossseed/domain_aliases.gointernal/services/crossseed/domain_aliases_test.gointernal/services/crossseed/hardlink_mode_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/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 (8)
- internal/api/handlers/dirscan_webhook_test.go
- internal/services/crossseed/domain_aliases.go
- internal/database/migrations/069_add_tracker_category_settings.sql
- internal/services/crossseed/tracker_category_test.go
- web/src/lib/api.ts
- internal/models/crossseed_indexer_categories_test.go
- internal/services/dirscan/inject_test.go
- internal/web/swagger/openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/services/dirscan/cancel_scan_test.go
- cmd/qui/main.go
- internal/services/crossseed/hardlink_mode_test.go
- internal/api/server_test.go
- internal/services/crossseed/auto_tmm_test.go
- internal/api/server.go
Collaborator
No need to rebase really, just merge develop into the branch whenever there's new changes. There is really no need to force push in PRs either, it just makes it harder to review them. We squash everything into a single commit when merging into develop, so PRs can be as messy as they have to 👍 Unless there's a different reason? 😄 |
Contributor
Author
|
👍 No other reasons here. Squashing is my normal workflow, but happy to add incremental commits and merge instead. Thanks! |
s0up4200
reviewed
Apr 9, 2026
s0up4200
left a comment
s0up4200
left a comment
Collaborator
There was a problem hiding this comment.
A couple questions here because I may be misunderstanding the intended behavior.
-
In
resolveDirscanTrackerCategory(...), if qBittorrent category lookup fails, dirscan still seems to keep the tracker-mapped category but continue without a save path / AutoTMM. Is that intentional? I expected this to fail, since the automation path appears to skip injects in that case. -
The same function only applies by-tracker mapping when hardlinks or reflinks are enabled. Was that meant to limit the feature to link mode only?
The PR description reads like per-indexer category resolution should apply generally, with link mode only changing path placement behavior.
If I’m reading it right, dirscan can either keep the mapped category but place files outside that category’s configured path when category lookup fails, or ignore tracker mappings entirely on by-tracker instances that aren’t using hardlinks/reflinks.
If that’s not intentional, both cases probably need tests.
Contributor
Author
|
Contributor
Author
|
Also worth noting - my setup exclusively uses hardlinks, so this feature path has been getting real-world testing for a while. Reflinks are untested on my end. |
added 2 commits
April 9, 2026 11:36
When the qBittorrent category lookup fails after a tracker category is resolved, return fatal=true so the inject is skipped rather than proceeding with hardlinks placed in the wrong location and no AutoTMM. Consistent with the crossseed service path behavior. Adds a test covering the fatal GetCategories error case.
Resolve conflict in ensureCrossCategory calls: adopt upstream's compareExistingSavePath bool parameter. Regular mode takes upstream verbatim (savePathToPersist distinction was unreachable). Hardlink mode keeps effectiveCategorySavePath with upstream's false flag.
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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)
10974-11458: 🛠️ Refactor suggestion | 🟠 MajorRefactor
processHardlinkModeinto smaller units to satisfy lint thresholds.Line 10974 begins a very long, branch-heavy function that now combines eligibility checks, path/category resolution, filesystem planning, add options, and recheck/resume handling. Please split it into focused helpers to reduce cognitive load and future regression risk.
♻️ Refactor shape (example)
-func (s *Service) processHardlinkMode(...) hardlinkModeResult { - // eligibility + base-dir resolution + category handling + plan build + add + recheck -} +func (s *Service) processHardlinkMode(...) hardlinkModeResult { + ctxData, early := s.prepareHardlinkContext(...) + if early != nil { + return *early + } + + pathPlan, err := s.resolveHardlinkPaths(...) + if err != nil { + return ctxData.handleError(err.Error()) + } + + if err := s.ensureHardlinkCategory(...); err != nil { + // existing fallback semantics + } + + plan, err := s.buildHardlinkPlan(...) + if err != nil { + return ctxData.handleError(err.Error()) + } + + options := s.buildHardlinkAddOptions(...) + return s.addHardlinkTorrentAndHandleResume(...) +}As per coding guidelines: “Use golangci-lint v2 with strict configuration targeting AI-generated code patterns, with funlen function length threshold at 80 lines” and “Use golangci-lint v2 with strict configuration targeting AI-generated code patterns, with 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/crossseed/service.go` around lines 10974 - 11458, The processHardlinkMode function is too large and should be split into smaller helpers: extract the eligibility and instance checks (including hardlinkError, handleError, instance.UseHardlinks, HasLocalFilesystemAccess, SkipRecheck logic) into validateHardlinkEligibility(ctx, candidate, instance, req) ; extract base dir and effectiveCategorySavePath resolution (the categorySavePath branch, FindMatchingBaseDir, buildCategorySavePath, effectiveCategorySavePath logic, and selectedBaseDir/destDir selection) into resolveHardlinkDirs(ctx, instance, matchedTorrent, props, categorySavePath, crossCategory, isTrackerCategoryMode, crossCategoryExistsInQbit, torrentHash, torrentName, incomingTrackerDomain, candidate, req) ; extract building file lists and candidate selection (candidateTorrentFilesAll, existingFiles, candidateTorrentFilesToLink, normalizeFileKey logic) into buildHardlinkFileLists(sourceFiles, candidateFiles, props) ; extract plan build/create/rollback and AddTorrent call (hardlinktree.BuildPlan, hardlinktree.Create, hardlinktree.Rollback, syncManager.AddTorrent) into executeHardlinkPlan(ctx, plan, candidate.InstanceID, torrentBytes, options) ; and extract recheck/queue logic (syncManager.BulkAction, queueRecheckResumeWithThreshold, queueRecheckResume, addPolicy handling) into handlePostAddRecheck(ctx, candidate.InstanceID, torrentHash, torrentHashV2, hasExtras, addPolicy, req). Update processHardlinkMode to orchestrate these helpers and return the same hardlinkModeResult without changing external behavior.
♻️ Duplicate comments (2)
internal/web/swagger/openapi.yaml (1)
3951-3977:⚠️ Potential issue | 🟡 MinorDocument the fallback semantics on the authenticated list endpoint too.
The proxy route now explains that missing indexers fall back to qui's normal category resolution, but the main
GET /api/cross-seed/indexer-categories/{instanceId}description still leaves that implicit. That makes the non-proxy contract ambiguous for callers that use session/API-key auth.Suggested diff
- description: Returns all per-indexer category mappings for a specific qBittorrent instance. When the "by-tracker" directory preset is active, these mappings override the global category for cross-seeds matched via each indexer. + description: | + Returns all per-indexer category mappings for a specific qBittorrent instance. + When the "by-tracker" directory preset is active, these mappings override the + global category for cross-seeds matched via each indexer. + + The response contains only explicit per-indexer overrides. If an indexer is not + present, callers should fall back to qui's normal category resolution order + (custom → indexer-name → affix → reuse).As per coding guidelines, API contract changes must update OpenAPI content under
internal/web/swaggerand passmake test-openapi.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/web/swagger/openapi.yaml` around lines 3951 - 3977, Update the OpenAPI description for the GET /api/cross-seed/indexer-categories/{instanceId} endpoint to explicitly document the fallback semantics: when an indexer is missing from the per-instance mapping the response does not include a mapping for that indexer and callers should expect the system to fall back to qBittorrent’s normal category resolution (same behavior as the proxy route). Edit the summary/description text under the path for this endpoint in internal/web/swagger/openapi.yaml (the GET /api/cross-seed/indexer-categories/{instanceId} entry referencing the CrossSeedIndexerCategory items) to state this fallback behavior, then run make test-openapi to validate the OpenAPI changes.web/src/pages/CrossSeedPage.tsx (1)
422-427:⚠️ Potential issue | 🟡 MinorSurface mapping load failures instead of rendering an empty list.
data: mappings = []still makes this panel look like “no mappings” while the query is pending or after it errors. Disabling Add mapping helps, but users still lose the distinction between “empty” and “failed to load”. Render a small loading/error state before the rows and only fall back to[]after a successful fetch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/CrossSeedPage.tsx` around lines 422 - 427, The query currently defaults data to an empty array (data: mappings = []), which hides loading/errors; remove the default so mappings can be undefined, and update the render logic around mappings, mappingsPending and mappingsError (from the useQuery call) to show a loading state when mappingsPending and an error state when mappingsError before rendering rows; only fall back to an empty array (e.g., const visibleMappings = mappings ?? []) after a successful fetch (!mappingsPending && !mappingsError). Also ensure the "Add mapping" control (the panel's Add mapping button) remains disabled while mappingsPending or mappingsError so users can distinguish empty vs failed-to-load states; references: useQuery(...), mappings, mappingsPending, mappingsError, api.getCrossSeedIndexerCategories, isExpanded.
🧹 Nitpick comments (2)
internal/services/crossseed/domain_aliases_test.go (1)
36-63: Good table-driven test coverage for alias mappings.The test validates that alias entries (Gazelle sites with obfuscated announce domains) correctly match their corresponding indexer names through the
MatchAnnounceDomainToIndexerfunction. As per coding guidelines, table-driven test patterns are preferred.Consider adding negative test cases (e.g.,
{announceDomain: "landof.tv", indexerName: "Redacted", want: false}) to verify that aliases don't incorrectly match unrelated indexers, but this is optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/crossseed/domain_aliases_test.go` around lines 36 - 63, The test currently covers positive alias mappings but lacks negative cases to ensure aliases don't match unrelated indexers; update TestTrackerDomainAliases to include table-driven negative tests (e.g., announceDomain "landof.tv" with indexerName "Redacted" expecting false, "flacsfor.me" with indexerName "Orpheus" expecting false, etc.) so MatchAnnounceDomainToIndexer is validated for non-matches as well; add those entries to the tests slice in internal/services/crossseed/domain_aliases_test.go near the existing cases.internal/services/dirscan/service.go (1)
1887-1888: Consider adding parentheses for clarity.The boolean expression is logically correct but relies on operator precedence (
&&binds tighter than||), which can be easy to misread:if !instance.UseHardlinks && !instance.UseReflinks || instance.HardlinkDirPreset != "by-tracker"Adding explicit parentheses would make the intent clearer for future readers:
♻️ Suggested readability improvement
- if !instance.UseHardlinks && !instance.UseReflinks || instance.HardlinkDirPreset != "by-tracker" { + if (!instance.UseHardlinks && !instance.UseReflinks) || instance.HardlinkDirPreset != "by-tracker" {🤖 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 1887 - 1888, The conditional using instance.UseHardlinks, instance.UseReflinks and instance.HardlinkDirPreset is correct but unclear due to operator precedence; update the if in service.go (the branch that checks !instance.UseHardlinks && !instance.UseReflinks || instance.HardlinkDirPreset != "by-tracker") to include explicit parentheses to show the intended grouping (e.g. parenthesize the && part or the || part) so readers immediately understand whether the check is "(!UseHardlinks && !UseReflinks) || HardlinkDirPreset != 'by-tracker'" or "!UseHardlinks && (!UseReflinks || HardlinkDirPreset != 'by-tracker')".
🤖 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_tracker_category_test.go`:
- Around line 21-31: The fakeDirscanCategoryGetter used in tests doesn't record
whether GetCategories was invoked, so
TestResolveDirscanTrackerCategory_FallsBackOnLookupError can pass without
proving qBittorrent was skipped; update fakeDirscanCategoryGetter to include a
call counter field (e.g., calls int or called bool) and increment it in
GetCategories, then in TestResolveDirscanTrackerCategory_FallsBackOnLookupError
assert the counter is 0 (ensuring the fallback path skipped calling qBittorrent)
and in the complementary test that expects GetCategories to run (e.g.,
TestResolveDirscanTrackerCategory_FatalOnGetCategories) assert the counter is 1;
reference fakeDirscanCategoryGetter.GetCategories and the two test names when
adding the counter and assertions.
---
Outside diff comments:
In `@internal/services/crossseed/service.go`:
- Around line 10974-11458: The processHardlinkMode function is too large and
should be split into smaller helpers: extract the eligibility and instance
checks (including hardlinkError, handleError, instance.UseHardlinks,
HasLocalFilesystemAccess, SkipRecheck logic) into
validateHardlinkEligibility(ctx, candidate, instance, req) ; extract base dir
and effectiveCategorySavePath resolution (the categorySavePath branch,
FindMatchingBaseDir, buildCategorySavePath, effectiveCategorySavePath logic, and
selectedBaseDir/destDir selection) into resolveHardlinkDirs(ctx, instance,
matchedTorrent, props, categorySavePath, crossCategory, isTrackerCategoryMode,
crossCategoryExistsInQbit, torrentHash, torrentName, incomingTrackerDomain,
candidate, req) ; extract building file lists and candidate selection
(candidateTorrentFilesAll, existingFiles, candidateTorrentFilesToLink,
normalizeFileKey logic) into buildHardlinkFileLists(sourceFiles, candidateFiles,
props) ; extract plan build/create/rollback and AddTorrent call
(hardlinktree.BuildPlan, hardlinktree.Create, hardlinktree.Rollback,
syncManager.AddTorrent) into executeHardlinkPlan(ctx, plan,
candidate.InstanceID, torrentBytes, options) ; and extract recheck/queue logic
(syncManager.BulkAction, queueRecheckResumeWithThreshold, queueRecheckResume,
addPolicy handling) into handlePostAddRecheck(ctx, candidate.InstanceID,
torrentHash, torrentHashV2, hasExtras, addPolicy, req). Update
processHardlinkMode to orchestrate these helpers and return the same
hardlinkModeResult without changing external behavior.
---
Duplicate comments:
In `@internal/web/swagger/openapi.yaml`:
- Around line 3951-3977: Update the OpenAPI description for the GET
/api/cross-seed/indexer-categories/{instanceId} endpoint to explicitly document
the fallback semantics: when an indexer is missing from the per-instance mapping
the response does not include a mapping for that indexer and callers should
expect the system to fall back to qBittorrent’s normal category resolution (same
behavior as the proxy route). Edit the summary/description text under the path
for this endpoint in internal/web/swagger/openapi.yaml (the GET
/api/cross-seed/indexer-categories/{instanceId} entry referencing the
CrossSeedIndexerCategory items) to state this fallback behavior, then run make
test-openapi to validate the OpenAPI changes.
In `@web/src/pages/CrossSeedPage.tsx`:
- Around line 422-427: The query currently defaults data to an empty array
(data: mappings = []), which hides loading/errors; remove the default so
mappings can be undefined, and update the render logic around mappings,
mappingsPending and mappingsError (from the useQuery call) to show a loading
state when mappingsPending and an error state when mappingsError before
rendering rows; only fall back to an empty array (e.g., const visibleMappings =
mappings ?? []) after a successful fetch (!mappingsPending && !mappingsError).
Also ensure the "Add mapping" control (the panel's Add mapping button) remains
disabled while mappingsPending or mappingsError so users can distinguish empty
vs failed-to-load states; references: useQuery(...), mappings, mappingsPending,
mappingsError, api.getCrossSeedIndexerCategories, isExpanded.
---
Nitpick comments:
In `@internal/services/crossseed/domain_aliases_test.go`:
- Around line 36-63: The test currently covers positive alias mappings but lacks
negative cases to ensure aliases don't match unrelated indexers; update
TestTrackerDomainAliases to include table-driven negative tests (e.g.,
announceDomain "landof.tv" with indexerName "Redacted" expecting false,
"flacsfor.me" with indexerName "Orpheus" expecting false, etc.) so
MatchAnnounceDomainToIndexer is validated for non-matches as well; add those
entries to the tests slice in internal/services/crossseed/domain_aliases_test.go
near the existing cases.
In `@internal/services/dirscan/service.go`:
- Around line 1887-1888: The conditional using instance.UseHardlinks,
instance.UseReflinks and instance.HardlinkDirPreset is correct but unclear due
to operator precedence; update the if in service.go (the branch that checks
!instance.UseHardlinks && !instance.UseReflinks || instance.HardlinkDirPreset !=
"by-tracker") to include explicit parentheses to show the intended grouping
(e.g. parenthesize the && part or the || part) so readers immediately understand
whether the check is "(!UseHardlinks && !UseReflinks) || HardlinkDirPreset !=
'by-tracker'" or "!UseHardlinks && (!UseReflinks || HardlinkDirPreset !=
'by-tracker')".
🪄 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: 2ed7b881-6321-4cb5-ba48-e7e231ae41f3
📒 Files selected for processing (30)
cmd/qui/main.gointernal/api/handlers/crossseed.gointernal/api/handlers/dirscan_webhook_test.gointernal/api/server.gointernal/api/server_test.gointernal/database/migrations/070_add_tracker_category_settings.sqlinternal/database/postgres_migrations/071_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/crossseed_test.gointernal/services/crossseed/domain_aliases.gointernal/services/crossseed/domain_aliases_test.gointernal/services/crossseed/hardlink_mode_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)
- web/src/types/index.ts
- internal/services/crossseed/domain_aliases.go
- cmd/qui/main.go
- internal/database/postgres_migrations/071_add_tracker_category_settings.sql
- internal/api/handlers/dirscan_webhook_test.go
- internal/services/crossseed/tracker_category_test.go
- internal/services/dirscan/inject_test.go
- internal/services/crossseed/crossseed_test.go
- internal/database/migrations/070_add_tracker_category_settings.sql
- internal/models/crossseed_indexer_categories_test.go
- internal/proxy/crossseed_indexer_categories_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/proxy/handler_test.go
- internal/services/dirscan/cancel_scan_test.go
- internal/services/crossseed/hardlink_mode_test.go
- internal/services/crossseed/auto_tmm_test.go
- internal/api/server.go
- web/src/lib/api.ts
- internal/services/dirscan/inject.go
- internal/api/handlers/crossseed.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/services/dirscan/service_tracker_category_test.go (1)
81-145: Prefer one table-driven test for these two error-path cases.Line 81 and Line 116 exercise the same function with near-identical flow. Consolidating into a table-driven test will reduce duplication and make it easier to extend coverage.
♻️ Suggested refactor
-func TestResolveDirscanTrackerCategory_FallsBackOnLookupError(t *testing.T) { - ... -} - -func TestResolveDirscanTrackerCategory_FatalOnGetCategoriesError(t *testing.T) { - ... -} +func TestResolveDirscanTrackerCategory_ErrorPaths(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + setupStore func(t *testing.T) (*models.CrossSeedIndexerCategoryStore, int) + getterErr error + wantFatal bool + wantCalls int + }{ + { + name: "fallback on lookup error", + setupStore: func(t *testing.T) (*models.CrossSeedIndexerCategoryStore, int) { + db, err := database.New(filepath.Join(t.TempDir(), "indexer_categories.db")) + require.NoError(t, err) + store := models.NewCrossSeedIndexerCategoryStore(db) + require.NoError(t, db.Close()) + return store, 7 + }, + getterErr: errors.New("should not be called"), + wantFatal: false, + wantCalls: 0, + }, + { + name: "fatal on GetCategories error", + setupStore: func(t *testing.T) (*models.CrossSeedIndexerCategoryStore, int) { + const instanceID = 42 + _, store := setupTrackerCategoryDB(t, instanceID, "Aither", "aither-movies") + return store, instanceID + }, + getterErr: errors.New("qbittorrent unavailable"), + wantFatal: true, + wantCalls: 1, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + store, instanceID := tc.setupStore(t) + logger := zerolog.New(io.Discard) + instance := &models.Instance{ + ID: instanceID, + UseHardlinks: true, + HardlinkDirPreset: "by-tracker", + } + + getter := &fakeDirscanCategoryGetter{err: tc.getterErr} + category, savePath, fatal := resolveDirscanTrackerCategory( + context.Background(), + instanceID, + "Aither", + "aither.cc", + instance, + store, + getter, + &logger, + ) + + require.Equal(t, tc.wantFatal, fatal) + require.Empty(t, category) + require.Empty(t, savePath) + require.Equal(t, tc.wantCalls, getter.calls) + }) + } +}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 duplicate tests into a single table-driven test that iterates cases for the lookup-error and get-categories-error paths: create a test table with fields (name, instanceID, prepopulateStore bool, getterErr, expectFatal, expectGetterCalls) and for each case set up the store either via New(dbPath) and close (for the lookup fallback case) or via setupTrackerCategoryDB (for the get-categories fatal case), construct the same instance struct, pass the fakeDirscanCategoryGetter with the desired err into resolveDirscanTrackerCategory, and assert fatal, category/savePath emptiness and getter.calls according to expectGetterCalls; reference resolveDirscanTrackerCategory, fakeDirscanCategoryGetter, setupTrackerCategoryDB and models.Instance when locating code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/services/dirscan/service_tracker_category_test.go`:
- Around line 81-145: Combine the two duplicate tests into a single table-driven
test that iterates cases for the lookup-error and get-categories-error paths:
create a test table with fields (name, instanceID, prepopulateStore bool,
getterErr, expectFatal, expectGetterCalls) and for each case set up the store
either via New(dbPath) and close (for the lookup fallback case) or via
setupTrackerCategoryDB (for the get-categories fatal case), construct the same
instance struct, pass the fakeDirscanCategoryGetter with the desired err into
resolveDirscanTrackerCategory, and assert fatal, category/savePath emptiness and
getter.calls according to expectGetterCalls; reference
resolveDirscanTrackerCategory, fakeDirscanCategoryGetter, setupTrackerCategoryDB
and models.Instance when locating code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 53d59352-e95b-4323-be05-bb755ef53799
📒 Files selected for processing (1)
internal/services/dirscan/service_tracker_category_test.go
- openapi: document fallback semantics on GET indexer-categories endpoint - dirscan: add parentheses to by-tracker guard for clarity - test(cross-seed): add negative alias cross-match cases
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/crossseed/domain_aliases_test.go`:
- Around line 18-25: The DFS in visit uses raw domain strings so
mixed-case/whitespace aliases can evade cycle detection; update visit to
normalize domains the same way MatchAnnounceDomainToIndexer does (use the same
normalization function) before the slices.Contains check and before looking up
trackerDomainAliases entries, and also normalize each alias when recursing (so
you compare and traverse normalizedDomain and normalizedAlias consistently).
🪄 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: e8c574bc-ac71-4165-a9df-b0aa5e33c762
📒 Files selected for processing (3)
internal/services/crossseed/domain_aliases_test.gointernal/services/dirscan/service.gointernal/web/swagger/openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/services/dirscan/service.go
- internal/web/swagger/openapi.yaml
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/services/crossseed/domain_aliases_test.go (1)
39-62: Consider adding normalization edge cases to the table.You already normalize in production logic; adding a couple mixed-case/whitespace alias cases here would harden regression coverage.
✅ Optional test additions
{ // BroadcasTheNet: announce via landof.tv, indexer named BroadcasTheNet {name: "BTN landof.tv matches BroadcasTheNet", announceDomain: "landof.tv", indexerName: "BroadcasTheNet", want: true}, {name: "BTN landof.tv matches BroadcasTheNet (Prowlarr)", announceDomain: "landof.tv", indexerName: "BroadcasTheNet (Prowlarr)", want: true}, + {name: "BTN LandOf.TV (trim/case) matches BroadcasTheNet", announceDomain: " LandOf.TV ", indexerName: "BroadcasTheNet", want: true}, // Redacted: announce via flacsfor.me, indexer named Redacted {name: "RED flacsfor.me matches Redacted", announceDomain: "flacsfor.me", indexerName: "Redacted", want: true}, {name: "RED flacsfor.me matches Redacted (Prowlarr)", announceDomain: "flacsfor.me", indexerName: "Redacted (Prowlarr)", want: true}, // Orpheus Network: announce via home.opsfet.ch, indexer named Orpheus / Orpheus Network {name: "OPS home.opsfet.ch matches Orpheus", announceDomain: "home.opsfet.ch", indexerName: "Orpheus", want: true}, {name: "OPS home.opsfet.ch matches Orpheus Network", announceDomain: "home.opsfet.ch", indexerName: "Orpheus Network", want: true}, {name: "OPS home.opsfet.ch matches Orpheus (Prowlarr)", announceDomain: "home.opsfet.ch", indexerName: "Orpheus (Prowlarr)", want: true}, + {name: "OPS home.opsfet.ch matches ORPHEUS NETWORK (trim/case)", announceDomain: "home.opsfet.ch", indexerName: " ORPHEUS NETWORK ", want: true},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/crossseed/domain_aliases_test.go` around lines 39 - 62, Add normalization edge-case rows to the tests slice in domain_aliases_test.go to ensure mixed-case and surrounding-whitespace aliases are covered; specifically append cases that use announceDomain and indexerName values with varied casing (e.g., "LandOf.TV" / "broadcasthenet") and with leading/trailing spaces (e.g., " flacsfor.me " / " Redacted (Prowlarr) ") and set want to the expected boolean, so the existing matching logic (the tests slice used by the test function that asserts domain-to-indexer aliasing) verifies normalization behavior.
🤖 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/domain_aliases_test.go`:
- Around line 39-62: Add normalization edge-case rows to the tests slice in
domain_aliases_test.go to ensure mixed-case and surrounding-whitespace aliases
are covered; specifically append cases that use announceDomain and indexerName
values with varied casing (e.g., "LandOf.TV" / "broadcasthenet") and with
leading/trailing spaces (e.g., " flacsfor.me " / " Redacted (Prowlarr) ")
and set want to the expected boolean, so the existing matching logic (the tests
slice used by the test function that asserts domain-to-indexer aliasing)
verifies normalization behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 654b8776-4029-4d46-aa09-f79fc877529d
📒 Files selected for processing (1)
internal/services/crossseed/domain_aliases_test.go
Restore the regular-mode safeguard for tracker-mapped categories so qui does not persist the matched torrent save path as the category save path. Adds a focused regression test for the save-path selection logic.
Contributor
There was a problem hiding this comment.
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)
8920-8964:⚠️ Potential issue | 🟠 MajorDrop the TLD-stripped fallback in specific-domain matching.
After the exact and dot-suffix checks, this branch still treats any same-label/different-TLD pair as equivalent. That can exclude the wrong indexer (
foo.ccvsfoo.net) and suppress valid matches during content filtering. Keep true cross-TLD aliases intrackerDomainAliasesinstead of broadening the specific-domain path.💡 Suggested change
if specificIndexerDomain != "" { for _, domain := range trackerDomains { normalizedDomain := normalizeLowerTrim(domain) normalizedSpecificDomain := normalizeLowerTrim(specificIndexerDomain) if normalizedDomain == normalizedSpecificDomain { log.Debug(). Str("matchType", "specific_indexer_domain_direct"). Str("torrentDomain", domain). Str("indexerDomain", specificIndexerDomain). Str("indexerName", indexerName). Msg("[CROSSSEED-DOMAIN] *** MATCH FOUND - Specific indexer domain direct match ***") return true } if strings.HasSuffix(normalizedDomain, "."+normalizedSpecificDomain) || strings.HasSuffix(normalizedSpecificDomain, "."+normalizedDomain) { log.Debug(). Str("matchType", "specific_indexer_domain_partial"). Str("torrentDomain", domain). Str("indexerDomain", specificIndexerDomain). Str("indexerName", indexerName). Msg("[CROSSSEED-DOMAIN] *** MATCH FOUND - Specific indexer domain partial match ***") return true } - - // TLD-stripped specific indexer domain comparison. - indexerDomainWithoutTLD := normalizedSpecificDomain - for _, suffix := range []string{".cc", ".org", ".net", ".com", ".to", ".me", ".tv", ".xyz"} { - if before, ok := strings.CutSuffix(indexerDomainWithoutTLD, suffix); ok { - indexerDomainWithoutTLD = before - break - } - } - domainWithoutTLD := normalizeDomainNameValue(stripTrackerTLD(normalizedDomain)) - normalizedIndexerDomainName := s.normalizeDomainName(indexerDomainWithoutTLD) - if domainWithoutTLD == normalizedIndexerDomainName { - log.Debug(). - Str("matchType", "normalized_specific_indexer_domain"). - Str("torrentDomain", domain). - Str("indexerDomain", specificIndexerDomain). - Str("indexerName", indexerName). - Msg("[CROSSSEED-DOMAIN] *** MATCH FOUND - Normalized specific indexer domain match ***") - return true - } } }🤖 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 8920 - 8964, The specific-indexer matching path should not include the TLD-stripped fallback: remove the block that computes indexerDomainWithoutTLD, iterates over suffixes (".cc", ".org", etc.), calls stripTrackerTLD/normalizeDomainName and compares domainWithoutTLD == s.normalizeDomainName(...), and the associated debug log and return; keep only the direct equality and strings.HasSuffix dot-suffix checks that use specificIndexerDomain and trackerDomains (functions/vars involved: specificIndexerDomain, trackerDomains, normalizeLowerTrim, strings.CutSuffix, stripTrackerTLD, s.normalizeDomainName, and the debug logging calls), so cross-TLD aliases are handled via trackerDomainAliases instead of this fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/services/crossseed/service.go`:
- Around line 8920-8964: The specific-indexer matching path should not include
the TLD-stripped fallback: remove the block that computes
indexerDomainWithoutTLD, iterates over suffixes (".cc", ".org", etc.), calls
stripTrackerTLD/normalizeDomainName and compares domainWithoutTLD ==
s.normalizeDomainName(...), and the associated debug log and return; keep only
the direct equality and strings.HasSuffix dot-suffix checks that use
specificIndexerDomain and trackerDomains (functions/vars involved:
specificIndexerDomain, trackerDomains, normalizeLowerTrim, strings.CutSuffix,
stripTrackerTLD, s.normalizeDomainName, and the debug logging calls), so
cross-TLD aliases are handled via trackerDomainAliases instead of this fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c7dc3b29-2ae8-45a5-89a5-d158b04a419d
📒 Files selected for processing (2)
internal/services/crossseed/category_persist_path_test.gointernal/services/crossseed/service.go
Limit specific-indexer-domain matching to direct equality and dot-suffix checks so cross-TLD aliases are handled only via trackerDomainAliases. Adds focused regression coverage for the removed fallback and the retained direct and subdomain cases.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/services/crossseed/domain_matching_test.go (1)
14-54: Refactor these into a single table-driven test.Line 14 through Line 54 repeat the same setup and differ only by input/expected output. Converting to a table-driven test will reduce duplication and align with repo test conventions.
♻️ Suggested refactor
-func TestTrackerDomainsMatchIndexer_DoesNotCrossMatchSpecificIndexerDomainAcrossTLD(t *testing.T) { - t.Parallel() - - service := &Service{ - jackettService: newFailingJackettService(errors.New("jackett lookup should not run in this test")), - indexerDomainCache: ttlcache.New(ttlcache.Options[string, string]{}), - domainMappings: trackerDomainAliases, - } - _ = service.indexerDomainCache.Set("My Indexer", "example.org", ttlcache.DefaultTTL) - - matched := service.trackerDomainsMatchIndexer([]string{"example.cc"}, "My Indexer") - require.False(t, matched) -} - -func TestTrackerDomainsMatchIndexer_MatchesSpecificIndexerDomainDirectly(t *testing.T) { - t.Parallel() - - service := &Service{ - jackettService: newFailingJackettService(errors.New("jackett lookup should not run in this test")), - indexerDomainCache: ttlcache.New(ttlcache.Options[string, string]{}), - domainMappings: trackerDomainAliases, - } - _ = service.indexerDomainCache.Set("My Indexer", "example.org", ttlcache.DefaultTTL) - - matched := service.trackerDomainsMatchIndexer([]string{"example.org"}, "My Indexer") - require.True(t, matched) -} - -func TestTrackerDomainsMatchIndexer_MatchesSpecificIndexerDomainBySubdomain(t *testing.T) { +func TestTrackerDomainsMatchIndexer_SpecificIndexerDomain(t *testing.T) { t.Parallel() - service := &Service{ - jackettService: newFailingJackettService(errors.New("jackett lookup should not run in this test")), - indexerDomainCache: ttlcache.New(ttlcache.Options[string, string]{}), - domainMappings: trackerDomainAliases, + tests := []struct { + name string + domains []string + expected bool + }{ + {name: "does not cross-match across TLD", domains: []string{"example.cc"}, expected: false}, + {name: "matches direct domain", domains: []string{"example.org"}, expected: true}, + {name: "matches subdomain", domains: []string{"tracker.example.org"}, expected: true}, } - _ = service.indexerDomainCache.Set("My Indexer", "example.org", ttlcache.DefaultTTL) - - matched := service.trackerDomainsMatchIndexer([]string{"tracker.example.org"}, "My Indexer") - require.True(t, matched) + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + service := &Service{ + jackettService: newFailingJackettService(errors.New("jackett lookup should not run in this test")), + indexerDomainCache: ttlcache.New(ttlcache.Options[string, string]{}), + domainMappings: trackerDomainAliases, + } + _ = service.indexerDomainCache.Set("My Indexer", "example.org", ttlcache.DefaultTTL) + + matched := service.trackerDomainsMatchIndexer(tc.domains, "My Indexer") + require.Equal(t, tc.expected, matched) + }) + } }As per coding guidelines:
**/*_test.go: “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/crossseed/domain_matching_test.go` around lines 14 - 54, Combine the three tests into a single table-driven test that iterates cases for input domains and expected result; create the shared setup once (instantiate Service with newFailingJackettService, ttlcache.New for indexerDomainCache, set indexerDomainCache.Set("My Indexer","example.org",...) and domainMappings = trackerDomainAliases) and then for each case call service.trackerDomainsMatchIndexer(case.domains, "My Indexer") and assert equals case.expected; keep references to Service, trackerDomainsMatchIndexer, indexerDomainCache.Set, trackerDomainAliases and newFailingJackettService so the refactor is a pure consolidation with identical assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/services/crossseed/service.go`:
- Around line 4445-4460: The code currently only aborts when category lookup
fails for useHardlinkMode, allowing by-tracker reflink adds to proceed without a
validated actualCategorySavePath; update the logic in the GetCategories error
branch (around syncManager.GetCategories, trackerCategoryMatched,
useHardlinkMode) to either (A) reject by-tracker reflink adds the same way as
hardlink mode by returning an InstanceCrossSeedResult with Status
"category_lookup_failed" and an explanatory Message when trackerCategoryMatched
&& !useHardlinkMode && reflink mode is requested, or (B) thread the confirmed
category/save-path state into processReflinkMode (add parameters like
actualCategorySavePath and a categoryLookupOk flag to processReflinkMode) and
make processReflinkMode apply autoTMM and savepath exactly as hardlink handling;
pick one approach and implement consistently so reflink adds cannot bypass
qBittorrent category save-path semantics.
---
Nitpick comments:
In `@internal/services/crossseed/domain_matching_test.go`:
- Around line 14-54: Combine the three tests into a single table-driven test
that iterates cases for input domains and expected result; create the shared
setup once (instantiate Service with newFailingJackettService, ttlcache.New for
indexerDomainCache, set indexerDomainCache.Set("My Indexer","example.org",...)
and domainMappings = trackerDomainAliases) and then for each case call
service.trackerDomainsMatchIndexer(case.domains, "My Indexer") and assert equals
case.expected; keep references to Service, trackerDomainsMatchIndexer,
indexerDomainCache.Set, trackerDomainAliases and newFailingJackettService so the
refactor is a pure consolidation with identical assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9b162401-abf6-4aa2-880a-da62034b2e56
📒 Files selected for processing (2)
internal/services/crossseed/domain_matching_test.gointernal/services/crossseed/service.go
Abort by-tracker adds on category lookup failure for both hardlink and reflink modes so reflinks cannot bypass qBittorrent category save-path semantics. Also consolidates the specific-indexer domain matching assertions into a single table-driven test.
Collaborator
|
@CjE5 I've decided to revert this for now. It introduced some unwanted behavior and I don't fully understand why, and the whole cross-seed part of qui is pretty complex and fragile. I appreciate the contribution, but for the time being I've decided to skip this. |
Contributor
Author
|
@s0up4200 No worries, sorry for the trouble. Any chance you could share the context from the channel? I think it's locked and I can't access it. If I can reproduce and fix whatever went wrong - with matching tests - would you be open to a re-submission? |
Contributor
Author
@s0up4200 binging this back to the top of your inbox. Any chance I can get the context from that channel? |
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.
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
Behavior Changes
API
Database
Tests