feat(cross-seed): scope onCompletion to instance #736
Conversation
|
Warning Rate limit exceeded@s0up4200 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds per-instance cross-seed completion persistence, API endpoints, DB migration, model helpers, service wiring to load/apply instance settings, frontend support (types, API client, UI) and comprehensive tests for the new store and helpers. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant API as CrossSeed Handler
participant Store as InstanceCompletionStore
participant DB as Database
participant Service as CrossSeed Service
rect rgba(76,175,80,0.08)
note over Client,API: GET per-instance settings
Client->>API: GET /cross-seed/completion/{instanceId}
API->>API: validate instanceId
API->>Store: Get(ctx, instanceId)
Store->>DB: SELECT ... FROM instance_crossseed_completion_settings
DB-->>Store: row / empty
Store-->>API: InstanceCrossSeedCompletionSettings (or defaults)
API-->>Client: 200 + JSON
end
rect rgba(33,150,243,0.08)
note over Client,API: PUT per-instance settings
Client->>API: PUT /cross-seed/completion/{instanceId} + payload
API->>API: validate instanceId & payload
API->>Store: Upsert(ctx, settings)
Store->>Store: sanitize & encode JSON
Store->>DB: INSERT ... ON CONFLICT DO UPDATE ...
DB-->>Store: ok
Store->>Store: Get(ctx, instanceId)
Store-->>API: updated settings
API-->>Client: 200 + JSON
end
rect rgba(255,193,7,0.08)
note over Service,Store: Background completion check
Service->>Store: Get(ctx, instanceId)
Store->>DB: SELECT ...
DB-->>Store: settings
Store-->>Service: settings
Service->>Service: matchesCompletionFilters(settings, torrent)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
835-917: Per‑instance completion gating breaks backward compatibility; migration lacks seeding logicThe new flow in
HandleTorrentCompletionloads instance‑scoped settings fromcompletionStore, falling back toDefaultInstanceCrossSeedCompletionSettings(instanceID)withEnabled == falsewhen no row exists. This correctly implements per-instance gating and cleanly separates instance completion from global automation settings.However, migration
029_add_instance_crossseed_completion_settings.sqlcreates an empty table without seeding any data from existingCrossSeedAutomationSettings.Completionsettings. On upgrade, any user with global completion enabled will see on-completion searches disabled for all instances until explicitly re-enabled per instance, sinceGet()returns defaults withEnabled == falsefor missing rows.Either seed
instance_crossseed_completion_settingsfrom global completion settings during migration/startup for each configured instance, or surface a clear warning in logs/UI that per-instance completion is now opt-in and defaults to disabled.
🧹 Nitpick comments (3)
web/src/types/index.ts (1)
68-75: Align TS type with backend if you plan to surfaceupdatedAtThe
InstanceCrossSeedCompletionSettingsinterface matches the core backend fields, but omitsupdatedAtwhich is present on the Go struct. If you intend to show “last updated” in the UI later, consider adding an optionalupdatedAt?: stringnow; otherwise this is fine as-is.internal/services/crossseed/service.go (1)
6256-6295: matchesCompletionFilters helper is generic and robust; only minor future tweaks possibleThe helper correctly:
- Treats nil torrent or settings as non‑match.
- Applies exclude‑first logic for categories and tags.
- Uses case‑insensitive matching for tags while keeping category comparisons exact (consistent with existing category handling elsewhere in this file).
If you ever decide that categories should be case‑insensitive too (to align with
SanitizeStringSlice’s lower‑casing behavior), you could mirror theEqualFoldpattern there, but that’s purely optional and not required for correctness.internal/models/instance_crossseed_completion.go (1)
109-165: Upsert + sanitization pipeline is solid; callers must ensure valid instance IDs
Upsert:
- Validates
settings != nil.- Clones and cleans all string slices through
sanitizeInstanceCrossSeedCompletionSettingsbefore encoding, ensuring trimmed, de‑duplicated values.- Uses
EncodeStringSliceJSONandBoolToSQLiteso the JSON/int columns stay normalized.- Performs a single
INSERT … ON CONFLICT(instance_id) DO UPDATEand then re‑loads viaGetto return the canonical stored record (includingUpdatedAt).This is a good pattern. The only expectation pushed to callers is that
InstanceIDis a valid, existing instance; if you want stricter guarantees you could add aninstanceID <= 0guard here later, but it’s not required for correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cmd/qui/main.go(2 hunks)internal/api/handlers/crossseed.go(4 hunks)internal/api/server.go(3 hunks)internal/database/migrations/029_add_instance_crossseed_completion_settings.sql(1 hunks)internal/models/crossseed.go(1 hunks)internal/models/helpers.go(1 hunks)internal/models/instance_crossseed_completion.go(1 hunks)internal/models/instance_reannounce.go(4 hunks)internal/services/crossseed/service.go(7 hunks)web/src/lib/api.ts(2 hunks)web/src/pages/CrossSeedPage.tsx(13 hunks)web/src/types/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: s0up4200
Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go:209-212
Timestamp: 2025-11-28T20:32:30.126Z
Learning: Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go
Learning: The cross-seed recheck-resume worker intentionally runs for the process lifetime and keys pending entries by hash only. This is acceptable under the current constraint that background seeded-search runs operate on a single instance at a time; graceful shutdown and instanceID|hash keying are deferred by design.
📚 Learning: 2025-11-28T20:32:30.126Z
Learnt from: s0up4200
Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go:209-212
Timestamp: 2025-11-28T20:32:30.126Z
Learning: Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go
Learning: The cross-seed recheck-resume worker intentionally runs for the process lifetime and keys pending entries by hash only. This is acceptable under the current constraint that background seeded-search runs operate on a single instance at a time; graceful shutdown and instanceID|hash keying are deferred by design.
Applied to files:
cmd/qui/main.gointernal/api/handlers/crossseed.gointernal/models/instance_crossseed_completion.gointernal/services/crossseed/service.go
📚 Learning: 2025-11-28T22:21:20.730Z
Learnt from: s0up4200
Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go:2415-2457
Timestamp: 2025-11-28T22:21:20.730Z
Learning: Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go
Learning: The determineSavePath function intentionally includes a contentLayout string parameter for future/content-layout branching and API consistency. Its presence is by design even if unused in the current body; do not flag as an issue in reviews.
Applied to files:
cmd/qui/main.gointernal/api/handlers/crossseed.gointernal/models/instance_crossseed_completion.gointernal/services/crossseed/service.go
📚 Learning: 2025-11-19T20:04:51.737Z
Learnt from: s0up4200
Repo: autobrr/qui PR: 606
File: internal/database/migrations/016_add_instance_reannounce_settings.sql:21-27
Timestamp: 2025-11-19T20:04:51.737Z
Learning: In the autobrr/qui repository, PRAGMA recursive_triggers is never enabled in the SQLite setup. The AFTER UPDATE trigger pattern that issues a separate UPDATE statement on the same table (e.g., to update an updated_at timestamp) is used consistently across migrations and does not cause recursion with recursive_triggers disabled (the SQLite default). This pattern should remain consistent across all migrations.
Applied to files:
internal/database/migrations/029_add_instance_crossseed_completion_settings.sql
📚 Learning: 2025-11-25T22:46:03.762Z
Learnt from: s0up4200
Repo: autobrr/qui PR: 632
File: internal/backups/service.go:1401-1404
Timestamp: 2025-11-25T22:46:03.762Z
Learning: In qui's backup service (internal/backups/service.go), background torrent downloads initiated during manifest import intentionally use a fire-and-forget pattern with the shared service context (s.ctx). Per-run cancellation is not needed, as orphaned downloads completing after run deletion are considered harmless and acceptable. This design prioritizes simplicity over per-run lifecycle management for background downloads.
Applied to files:
internal/services/crossseed/service.go
🧬 Code graph analysis (8)
cmd/qui/main.go (1)
internal/models/instance_crossseed_completion.go (2)
NewInstanceCrossSeedCompletionStore(33-35)InstanceCrossSeedCompletionStore(28-30)
internal/models/crossseed.go (1)
web/src/types/index.ts (1)
CrossSeedCompletionSettings(1403-1409)
web/src/types/index.ts (1)
internal/models/instance_crossseed_completion.go (1)
InstanceCrossSeedCompletionSettings(17-25)
internal/models/instance_reannounce.go (1)
internal/models/helpers.go (4)
EncodeStringSliceJSON(44-53)BoolToSQLite(13-18)SanitizeStringSlice(22-41)DecodeStringSliceJSON(57-66)
internal/api/handlers/crossseed.go (3)
internal/models/instance_crossseed_completion.go (3)
InstanceCrossSeedCompletionStore(28-30)DefaultInstanceCrossSeedCompletionSettings(53-62)InstanceCrossSeedCompletionSettings(17-25)internal/api/handlers/helpers.go (2)
RespondError(34-38)RespondJSON(22-31)web/src/types/index.ts (1)
InstanceCrossSeedCompletionSettings(68-75)
internal/models/instance_crossseed_completion.go (3)
internal/dbinterface/querier.go (1)
Querier(30-35)web/src/types/index.ts (1)
InstanceCrossSeedCompletionSettings(68-75)internal/models/helpers.go (4)
EncodeStringSliceJSON(44-53)BoolToSQLite(13-18)SanitizeStringSlice(22-41)DecodeStringSliceJSON(57-66)
web/src/pages/CrossSeedPage.tsx (3)
web/src/lib/api.ts (1)
api(1711-1711)internal/models/instance_crossseed_completion.go (1)
InstanceCrossSeedCompletionSettings(17-25)web/src/types/index.ts (1)
InstanceCrossSeedCompletionSettings(68-75)
internal/api/server.go (2)
internal/models/instance_crossseed_completion.go (1)
InstanceCrossSeedCompletionStore(28-30)internal/api/handlers/crossseed.go (1)
NewCrossSeedHandler(277-282)
🔇 Additional comments (19)
internal/database/migrations/029_add_instance_crossseed_completion_settings.sql (1)
4-20: Instance completion settings table and trigger look consistent with existing patternsSchema (PK/FK, JSON defaults) and the AFTER UPDATE trigger on
updated_atalign with the established migration style in this repo and should behave correctly given recursive triggers remain disabled.web/src/lib/api.ts (1)
39-40: Client wiring for per‑instance completion settings is consistentThe new
getInstanceCompletionSettingsandupdateInstanceCompletionSettingsmethods correctly use the sharedInstanceCrossSeedCompletionSettingstype and the/cross-seed/completion/{instanceId}endpoints, with the update payload excludinginstanceIdas expected by the handler.Also applies to: 992-1004
internal/models/helpers.go (1)
12-66: Shared helpers for bool/JSON/string slices are well-factoredCentralizing SQLite bool conversion and string-slice sanitization/JSON handling here improves consistency and avoids per-model duplication; the nil/empty cases are handled safely.
cmd/qui/main.go (1)
546-550: Instance completion store wiring into service and server looks correct
instanceCrossSeedCompletionStoreis constructed once from the main DB handle, passed intocrossseed.NewService, and exposed viaapi.Dependencies.InstanceCrossSeedCompletionStore, so the per-instance settings are available to both the service layer and HTTP handlers without altering startup/shutdown behavior.Also applies to: 631-657
internal/models/crossseed.go (1)
62-82: CompletionFilterProvider abstraction cleanly decouples filter consumers from concrete settings typesDefining
CompletionFilterProviderand havingCrossSeedCompletionSettingsimplement it is a straightforward way to allow shared matching logic over both global and instance-scoped completion settings.internal/api/handlers/crossseed.go (1)
25-28: Per-instance completion endpoints are wired and validated consistentlyThe added
completionStoredependency,/cross-seed/completion/{instanceID}routes, and theGetInstanceCompletionSettings/UpdateInstanceCompletionSettingshandlers follow existing patterns (instanceID validation, JSON decode, structured error responses). The nil-store fallback on GET (default settings + warning) and explicit 503 on PUT give a reasonable behavior gradient if the store is ever unset.Also applies to: 276-282, 289-315, 1102-1205
internal/models/instance_reannounce.go (1)
128-177: Reannounce settings now correctly reuse shared helpersSwitching to
EncodeStringSliceJSON/DecodeStringSliceJSON,SanitizeStringSlice, andBoolToSQLitekeeps the existing behavior (trimmed, deduped slices; 0/1 booleans) while consolidating the implementation inhelpers.go, improving maintainability.Also applies to: 209-211, 256-267
web/src/pages/CrossSeedPage.tsx (5)
30-35: LGTM!Import changes correctly reflect the transition from global to per-instance completion settings.
310-317: LGTM!The per-instance completion settings query is correctly guarded by the
enabledcondition, making the non-null assertion safe.
493-503: LGTM!The mutation correctly updates the query cache using the response's
instanceId, ensuring cache consistency when switching between instances.
564-580: LGTM!The save handler correctly validates instance selection and constructs the payload with parsed list values.
1251-1286: LGTM!The instance selector UI is well-implemented with appropriate disabled states and loading indicators. The flow of resetting
completionFormInitializedwhen changing instances ensures the form properly reinitializes with the new instance's settings.internal/services/crossseed/service.go (2)
235-351: Service wiring for per‑instance completion store looks consistentThe new
completionStore *models.InstanceCrossSeedCompletionStorefield andNewServiceparameter/assignment are correctly threaded through; no lifecycle or concurrency issues stand out here.
894-901: Completion filters now bound to per‑instance settings – semantics look correct
matchesCompletionFilters(&torrent, completionSettings)now uses the instance’sCompletionFilterProviderrather than the former global completion block. The include/exclude logic:
- Excludes on category if in
GetExcludeCategories().- Optionally whitelists on
GetCategories().- Excludes on tags via case‑insensitive match against
GetExcludeTags().- If
GetTags()is non‑empty, requires at least one case‑insensitive tag match to pass.This matches the typical “exclude, then include” semantics and should behave predictably for both global and instance implementations of
CompletionFilterProvider.internal/models/instance_crossseed_completion.go (3)
16-62: Per‑instance completion settings model is well‑shaped and interface‑ready
InstanceCrossSeedCompletionSettings:
- Matches the frontend interface shape (
instanceId,enabled,categories,tags,excludeCategories,excludeTags,updatedAt).- Implements the expected filter getter methods so it can be used polymorphically as a
CompletionFilterProvider.- Provides a clear
DefaultInstanceCrossSeedCompletionSettingswithEnabled=false, which aligns with the safety‑first behavior you’re enforcing in the service.Struct layout and defaults look correct.
64-107: Get/List store methods correctly handle defaults and scanningThe
GetandListmethods:
- Use a shared
scanInstanceCrossSeedCompletionSettingshelper for both*sql.Rowand*sql.Rowsvia the small scanner interface.- Correctly treat
sql.ErrNoRowsas “no override”, returningDefaultInstanceCrossSeedCompletionSettings(instanceID)without error.- Return only instances that have explicit overrides from
List, which is appropriate when callers know how to supply defaults for missing rows.No issues here; the behavior matches the comment and fits how the service uses these settings.
167-223: scanInstanceCrossSeedCompletionSettings correctly decodes and surfaces errorsThe scan helper:
- Reads
enabledas anintand maps1→true, which matchesBoolToSQLite.- Uses
DecodeStringSliceJSONfor all JSON columns and wraps each with context ("decode categories", etc.) making driver‑level issues diagnosable.- Handles a nullable
updated_atand leaves it at the zero value when not set.Everything here looks consistent with the helpers and the migration layout.
internal/api/server.go (2)
49-137: API server dependency wiring for instance completion store is consistentThe
ServerandDependenciesstructs, plusNewServer, now consistently carryInstanceCrossSeedCompletionStorethrough to the server:
Dependencies.InstanceCrossSeedCompletionStore→Server.instanceCrossSeedCompletionStore.- No changes to existing dependencies or timeouts.
This looks correct; just ensure that all construction sites for
Dependenciesare updated to provide a non‑nil store when cross‑seed is enabled.
271-322: Cross‑seed handler now has direct access to per‑instance completion store
crossSeedHandler := handlers.NewCrossSeedHandler(s.crossSeedService, s.instanceCrossSeedCompletionStore)cleanly injects the new store into the API layer so per‑instance completion settings can be managed over HTTP.The route registration (
crossSeedHandler.Routes(r)) is otherwise unchanged, so this should be a non‑breaking extension of the existing API surface as long as the handler defends against a nil store where appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/web/swagger/openapi.yaml (1)
2694-2695: Clarify the 503 store-not-configured error condition.The 503 error "Completion settings store not configured" references an operational/configuration dependency. Consider documenting in a comment or related issue what this condition means and when it occurs (e.g., missing database tables, misconfigured storage backend).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/web/swagger/openapi.yaml(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: s0up4200
Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go:209-212
Timestamp: 2025-11-28T20:32:30.126Z
Learning: Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go
Learning: The cross-seed recheck-resume worker intentionally runs for the process lifetime and keys pending entries by hash only. This is acceptable under the current constraint that background seeded-search runs operate on a single instance at a time; graceful shutdown and instanceID|hash keying are deferred by design.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run tests
🔇 Additional comments (2)
internal/web/swagger/openapi.yaml (2)
2641-2696: Well-structured API endpoints for per-instance cross-seed completion settings.The new endpoints properly scope the
onCompletionfunctionality to individual instances. The GET and PUT operations follow RESTful conventions, and error responses are appropriate for the use cases (invalid instance ID, store-not-configured scenarios).
3824-3882: Schemas correctly distinguish between response and request payloads.
InstanceCrossSeedCompletionSettingsappropriately includes theinstanceIdfield in the response, whileInstanceCrossSeedCompletionSettingsRequestcorrectly omits it since the instance ID is already provided via the path parameter. Field descriptions are clear and document the filtering semantics.
- Return 503 instead of silent fallback when completion store is nil - Validate instance existence before get/update completion settings - Add nil check in NewInstanceCrossSeedCompletionStore constructor - Extract toInstanceCompletionSettingsResponse helper to reduce duplication - Standardize interface receiver types to pointer receivers - Add unit tests for helpers and completion store
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/models/instance_crossseed_completion.go (1)
112-159: Consider validatingInstanceIDbefore upserting
Upsertassumessettings.InstanceIDis valid and defers any validation to the DB layer. Given this is per‑instance configuration, you may want to explicitly rejectInstanceID <= 0up front (similar to other stores that validate positive IDs) to fail fast before hitting the database. Not critical if callers already guarantee a valid instance, but it would harden the API surface a bit.internal/api/server.go (1)
74-101:Dependencies.WebHandleris unused; consider removing or actually injecting itThe
Dependenciesstruct now includesWebHandler *web.Handler, butServerignores it and constructs a new web handler viaweb.NewHandler(...)insideHandler(). That’s harmless, but a bit misleading for callers. Either use the injectedWebHandler(for better testability and clearer DI) or drop the field fromDependenciesto avoid confusion.Also applies to: 505-519
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/api/handlers/crossseed.go(4 hunks)internal/api/server.go(3 hunks)internal/models/crossseed.go(1 hunks)internal/models/helpers_test.go(1 hunks)internal/models/instance_crossseed_completion.go(1 hunks)internal/models/instance_crossseed_completion_test.go(1 hunks)internal/services/crossseed/service.go(7 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: s0up4200
Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go:209-212
Timestamp: 2025-11-28T20:32:30.126Z
Learning: Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go
Learning: The cross-seed recheck-resume worker intentionally runs for the process lifetime and keys pending entries by hash only. This is acceptable under the current constraint that background seeded-search runs operate on a single instance at a time; graceful shutdown and instanceID|hash keying are deferred by design.
📚 Learning: 2025-11-28T20:32:30.126Z
Learnt from: s0up4200
Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go:209-212
Timestamp: 2025-11-28T20:32:30.126Z
Learning: Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go
Learning: The cross-seed recheck-resume worker intentionally runs for the process lifetime and keys pending entries by hash only. This is acceptable under the current constraint that background seeded-search runs operate on a single instance at a time; graceful shutdown and instanceID|hash keying are deferred by design.
Applied to files:
internal/models/instance_crossseed_completion_test.gointernal/api/handlers/crossseed.gointernal/services/crossseed/service.gointernal/models/instance_crossseed_completion.go
📚 Learning: 2025-11-28T22:21:20.730Z
Learnt from: s0up4200
Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go:2415-2457
Timestamp: 2025-11-28T22:21:20.730Z
Learning: Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go
Learning: The determineSavePath function intentionally includes a contentLayout string parameter for future/content-layout branching and API consistency. Its presence is by design even if unused in the current body; do not flag as an issue in reviews.
Applied to files:
internal/api/handlers/crossseed.gointernal/services/crossseed/service.gointernal/models/instance_crossseed_completion.go
📚 Learning: 2025-11-25T22:46:03.762Z
Learnt from: s0up4200
Repo: autobrr/qui PR: 632
File: internal/backups/service.go:1401-1404
Timestamp: 2025-11-25T22:46:03.762Z
Learning: In qui's backup service (internal/backups/service.go), background torrent downloads initiated during manifest import intentionally use a fire-and-forget pattern with the shared service context (s.ctx). Per-run cancellation is not needed, as orphaned downloads completing after run deletion are considered harmless and acceptable. This design prioritizes simplicity over per-run lifecycle management for background downloads.
Applied to files:
internal/services/crossseed/service.go
🧬 Code graph analysis (6)
internal/models/helpers_test.go (1)
internal/models/helpers.go (4)
BoolToSQLite(13-18)SanitizeStringSlice(22-41)EncodeStringSliceJSON(44-53)DecodeStringSliceJSON(57-66)
internal/models/instance_crossseed_completion_test.go (2)
internal/models/instance_crossseed_completion.go (3)
NewInstanceCrossSeedCompletionStore(33-38)InstanceCrossSeedCompletionSettings(17-25)DefaultInstanceCrossSeedCompletionSettings(56-65)internal/models/crossseed.go (1)
CompletionFilterProvider(64-69)
internal/api/server.go (2)
internal/models/instance_crossseed_completion.go (1)
InstanceCrossSeedCompletionStore(28-30)internal/api/handlers/crossseed.go (1)
NewCrossSeedHandler(278-284)
internal/api/handlers/crossseed.go (3)
internal/models/instance_crossseed_completion.go (2)
InstanceCrossSeedCompletionStore(28-30)InstanceCrossSeedCompletionSettings(17-25)internal/models/instance.go (1)
InstanceStore(127-130)internal/api/handlers/helpers.go (2)
RespondError(34-38)RespondJSON(22-31)
internal/services/crossseed/service.go (2)
internal/models/instance_crossseed_completion.go (1)
InstanceCrossSeedCompletionStore(28-30)internal/models/crossseed.go (1)
CompletionFilterProvider(64-69)
internal/models/instance_crossseed_completion.go (3)
internal/dbinterface/querier.go (1)
Querier(30-35)web/src/types/index.ts (1)
InstanceCrossSeedCompletionSettings(68-75)internal/models/helpers.go (4)
EncodeStringSliceJSON(44-53)BoolToSQLite(13-18)SanitizeStringSlice(22-41)DecodeStringSliceJSON(57-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run tests
🔇 Additional comments (9)
internal/models/helpers_test.go (1)
13-197: Helper tests are thorough and correctly mirror helper semanticsThese table-driven tests exercise the boolean mapping, slice sanitization (trim, empty removal, case-insensitive dedup, order), JSON encoding edge cases (nil/empty, special chars), and decoding (nulls, whitespace, invalid JSON, and sanitization). They align well with the helpers in
internal/models/helpers.goand should catch regressions in behavior, especially around sanitization. No issues from my side here.internal/models/crossseed.go (1)
62-82: CompletionFilterProvider interface and getters look appropriateThe interface cleanly abstracts the four filter slices, and the pointer-receiver getters on
CrossSeedCompletionSettingsare straightforward and consistent with the new per‑instance settings type. As long as call sites always pass non‑nil pointers (which is the normal pattern here), this is a solid way to share matching logic across global and per‑instance completion settings.internal/models/instance_crossseed_completion.go (1)
16-65: Per-instance completion settings store is coherent and matches the helpersThe model + store design is consistent:
Getcorrectly falls back to defaults onsql.ErrNoRows,Upsertclones and sanitizes viaSanitizeStringSliceandEncodeStringSliceJSONbefore persisting, andscanInstanceCrossSeedCompletionSettingsusesDecodeStringSliceJSONto reapply the same sanitization on read. UsingBoolToSQLitefor theenabledflag keeps the schema simple and predictable. Overall this is a solid persistence layer for per‑instance completion filters.Also applies to: 112-159, 161-168
internal/models/instance_crossseed_completion_test.go (1)
18-255: Store and interface tests give very good coverageThese tests hit all the important behaviors: constructor panic on nil DB, default fallbacks, insert vs update via
Upsert, sanitization semantics,Listaggregation, default constructor behavior, andCompletionFilterProviderconformance. TheinsertTestInstancehelper also ensures realistic FK usage. This should make refactors to the per‑instance store quite safe.internal/api/server.go (1)
49-72: Cross-seed handler wiring is correct and properly injected
ServercarriesinstanceCrossSeedCompletionStoreand passes it through the DI chain: constructed incmd/qui/main.go:548, included inDependencies, assigned toServerinNewServer, and used inHandler()at line 254 wherecrossSeedHandler := handlers.NewCrossSeedHandler(s.crossSeedService, s.instanceCrossSeedCompletionStore, s.instanceStore)is called. The store is always non-nil due to the panic check inNewInstanceCrossSeedCompletionStore, so no defensive checks are needed downstream.One minor note:
Dependencies.WebHandlerfield (line 87) is unused and could be removed as cleanup, though it's not blocking anything.internal/services/crossseed/service.go (3)
236-290: Service wiring forcompletionStorelooks consistentInjecting
*models.InstanceCrossSeedCompletionStoreintoServiceviaNewServiceand storing it on the struct is consistent with the existing dependency pattern and keeps per-instance completion concerns encapsulated. No issues here from a construction / lifecycle perspective.Also applies to: 300-309, 324-337
6252-6291: Interface-basedmatchesCompletionFiltersis fine; be aware of typed-nil providersThe generalized
matchesCompletionFilterscorrectly preserves the previous filtering semantics (exclude categories → include categories → exclude tags → include tags) and works cleanly with the newCompletionFilterProviderinterface.One behavioral edge case to keep in mind: if a typed nil provider (e.g.
(*InstanceCrossSeedCompletionSettings)(nil)) is ever passed in,settings == nilwill be false and method calls will panic. Given the current call site always expects a non‑nil settings object, this is fine, but it reinforces the value of the extra nil guard suggested aroundcompletionStore.GetinHandleTorrentCompletion.Aside from that caveat, the function is clear and idiomatic.
835-923: Remove the suggested nil check—the store contract guarantees non-nil on nil errorThe
InstanceCrossSeedCompletionStore.Get()implementation explicitly handles missing records by returningDefaultInstanceCrossSeedCompletionSettings(instanceID), nilwhen no row is found. On any other error, it returns(nil, err). This means the store never returns(nil, nil), making the suggested nil guard unnecessary.The existing code is safe as written.
internal/api/handlers/crossseed.go (1)
26-29: Handler wiring and routing for per-instance completion are coherentAdding
completionStoreandinstanceStoretoCrossSeedHandler, threading them throughNewCrossSeedHandler, and exposing/api/cross-seed/completion/{instanceID}routes fits cleanly into the existing handler pattern. The separation between global automation endpoints and per-instance completion endpoints is clear and discoverable.Also applies to: 278-283, 291-317
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/services/crossseed/service.go (2)
236-290:completionStoreis effectively required; consider making this explicit or soft‑failing
Service.HandleTorrentCompletiontreats a nilcompletionStoreas an error and bails early on every completion event, which means any caller that constructsServicewithcompletionStore == nil(tests, legacy wiring, CLI tools) will silently disable completion and emit error logs.Two possible cleanups:
- Make
completionStorea required dependency:
- Keep the pointer type but document it as non‑nil and panic early in
NewServicewhen nil, or- Accept
*models.InstanceCrossSeedCompletionStoreinNewServicebut assign to a non‑pointer field.- Or, treat
nilas “completion disabled”:
- Downgrade the log in
HandleTorrentCompletionto debug/info and don’t consider it an error condition.Either path makes the dependency contract clearer and avoids surprising error‑level log spam if a caller intentionally omits per‑instance completion.
Also applies to: 300-337
848-912: Defensively handle a nilcompletionSettingsand consider a bounded DB call here
completionSettings, err := s.completionStore.Get(ctx, instanceID)is assumed non‑nil;completionSettings.Enabledwill panic if the store ever returns(nil, nil)for a missing row or default state.If
InstanceCrossSeedCompletionStore.Getdoes not hard‑guarantee a non‑nil settings struct, it would be safer to guard:completionSettings, err := s.completionStore.Get(ctx, instanceID) if err != nil { // existing warn + return return } + if completionSettings == nil { + log.Warn(). + Int("instanceID", instanceID). + Str("hash", torrent.Hash). + Msg("[CROSSSEED-COMPLETION] Instance completion settings missing; skipping completion search") + return + }Also, unlike
GetAutomationSettings, this DB lookup runs on the passedctxwithout its own timeout; if the completion store blocks, it stalls the completion hook path. Mirroring the small, bounded timeout used for automation settings (or a short per‑call timeout here) would keep this path resilient under DB hiccups.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/services/crossseed/service.go(7 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: s0up4200
Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go:209-212
Timestamp: 2025-11-28T20:32:30.126Z
Learning: Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go
Learning: The cross-seed recheck-resume worker intentionally runs for the process lifetime and keys pending entries by hash only. This is acceptable under the current constraint that background seeded-search runs operate on a single instance at a time; graceful shutdown and instanceID|hash keying are deferred by design.
📚 Learning: 2025-11-28T20:32:30.126Z
Learnt from: s0up4200
Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go:209-212
Timestamp: 2025-11-28T20:32:30.126Z
Learning: Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go
Learning: The cross-seed recheck-resume worker intentionally runs for the process lifetime and keys pending entries by hash only. This is acceptable under the current constraint that background seeded-search runs operate on a single instance at a time; graceful shutdown and instanceID|hash keying are deferred by design.
Applied to files:
internal/services/crossseed/service.go
📚 Learning: 2025-11-28T22:21:20.730Z
Learnt from: s0up4200
Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go:2415-2457
Timestamp: 2025-11-28T22:21:20.730Z
Learning: Repo: autobrr/qui PR: 641
File: internal/services/crossseed/service.go
Learning: The determineSavePath function intentionally includes a contentLayout string parameter for future/content-layout branching and API consistency. Its presence is by design even if unused in the current body; do not flag as an issue in reviews.
Applied to files:
internal/services/crossseed/service.go
📚 Learning: 2025-11-25T22:46:03.762Z
Learnt from: s0up4200
Repo: autobrr/qui PR: 632
File: internal/backups/service.go:1401-1404
Timestamp: 2025-11-25T22:46:03.762Z
Learning: In qui's backup service (internal/backups/service.go), background torrent downloads initiated during manifest import intentionally use a fire-and-forget pattern with the shared service context (s.ctx). Per-run cancellation is not needed, as orphaned downloads completing after run deletion are considered harmless and acceptable. This design prioritizes simplicity over per-run lifecycle management for background downloads.
Applied to files:
internal/services/crossseed/service.go
🧬 Code graph analysis (1)
internal/services/crossseed/service.go (2)
internal/models/instance_crossseed_completion.go (1)
InstanceCrossSeedCompletionStore(28-30)internal/models/crossseed.go (2)
DefaultCrossSeedAutomationSettings(96-122)CompletionFilterProvider(64-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run tests
🔇 Additional comments (1)
internal/services/crossseed/service.go (1)
6365-6404: Completion filter helper looks correct and generalizes cleanly to the interfaceThe new
matchesCompletionFilterscorrectly:
- Short‑circuits on exclude categories/tags,
- Enforces inclusive categories when configured,
- Requires at least one allowed tag when
GetTags()is non‑empty, and- Falls back to “allow” when no positive filters are set.
The
CompletionFilterProviderabstraction fits both global and per‑instance settings without leaking concrete types, and the tag comparisons are case‑insensitive as expected. No functional issues spotted here.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.