feat(tracker-rules): add per-tracker limits and services page#630
feat(tracker-rules): add per-tracker limits and services page#630
Conversation
- Add Tracker Rules panel to configure per-tracker speed, ratio, and seeding limits per instance. - Introduce a Services page that surfaces Tracker Rules alongside Tracker Reannounce for the selected instance. - Move Tracker Reannounce out of the Instance Preferences dialog into the Services page and refine its UX (Aggressive mode copy, enable/disable behavior, instance scoping).
WalkthroughAdds a Tracker Rules feature: DB migration and model/store, API handlers and server wiring, a background service that applies rules to torrents, frontend types/client/UI (Tracker Rules panel and Services page), and reannounce behavior/UI updates (Quick Retry and initial-wait enforcement). Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Web as Web UI
participant API as API Server
participant Store as TrackerRuleStore
participant Service as TrackerRuleService
participant QBT as qBittorrent
User->>Web: Open Services page / select instance
Web->>API: GET /api/instances/{id}/tracker-rules
API->>Store: ListByInstance(instanceID)
Store-->>API: [TrackerRule]
API-->>Web: 200 [TrackerRule]
User->>Web: Create/Update rule
Web->>API: POST/PUT /api/instances/{id}/tracker-rules
API->>Store: Create/Update(rule)
Store-->>API: TrackerRule
API-->>Web: 201/200 TrackerRule
User->>Web: Click "Apply Now"
Web->>API: POST /api/instances/{id}/tracker-rules/apply
API->>Service: ApplyOnceForInstance(instanceID)
Service->>Store: ListByInstance(instanceID)
Store-->>Service: [TrackerRule]
Service->>QBT: Fetch torrents, compute diffs
Service->>QBT: Apply limits (batched)
QBT-->>Service: Success
Service-->>API: OK
API-->>Web: 200 OK
rect rgba(180,230,200,0.12)
Note over Service: Background loop periodically runs applyAll()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 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/reannounce/service.go (1)
284-284: Inconsistent debounce window usage in GetMonitoredTorrents.Line 284 uses the fixed
s.cfg.DebounceWindowto determine cooldown state, but the enqueue method now uses a dynamic window (ReannounceIntervalSeconds in aggressive mode). This inconsistency will cause GetMonitoredTorrents to report incorrect cooldown states for torrents when aggressive mode is enabled.The same helper logic used in enqueue (lines 341-350) should be applied here to determine the correct debounce window per instance settings.
s.jobsMu.Lock() defer s.jobsMu.Unlock() now := s.currentTime() instJobs := s.j[instanceID] + + // Determine debounce window matching enqueue logic + debounceWindow := s.cfg.DebounceWindow + if settings != nil && settings.Aggressive { + if interval := time.Duration(settings.ReannounceIntervalSeconds) * time.Second; interval > 0 { + debounceWindow = interval + } + } var result []MonitoredTorrent for _, torrent := range torrents { if !s.torrentMeetsCriteria(torrent, settings) { continue } hasProblem := s.trackerProblemDetected(torrent.Trackers) waiting := s.trackersUpdating(torrent.Trackers) && !hasProblem if !hasProblem && !waiting { continue } hashUpper := strings.ToUpper(strings.TrimSpace(torrent.Hash)) if hashUpper == "" { continue } state := MonitoredTorrentStateWatching if instJobs != nil { if job, ok := instJobs[hashUpper]; ok { if job.isRunning { state = MonitoredTorrentStateReannouncing - } else if !job.lastCompleted.IsZero() && now.Sub(job.lastCompleted) < s.cfg.DebounceWindow { + } else if !job.lastCompleted.IsZero() && now.Sub(job.lastCompleted) < debounceWindow { state = MonitoredTorrentStateCooldown } } }web/src/components/instances/preferences/TrackerReannounceForm.tsx (1)
124-170: Centralized persistence looks good; copy still assumes dialog contextThe new
persistSettingsflow withsanitizeSettingsand the consolidated submit/toggle behavior is clean and reuses the existingupdateInstanceAPI correctly. Two small UX nits:
- Both the early-return error (
toast.error("Instance missing", ...)) and the fallback render ("Please close and reopen the dialog.") still mention a “dialog”, which is confusing when this form is rendered on the Services page. Consider rephrasing to something like “Please reload the page and try again.” or a more generic message.- Disabling monitoring persists immediately, while enabling requires hitting “Save Changes”. That asymmetry is easy to miss; you might want to either persist both directions on toggle or add a short hint near the switch/card to clarify that enabling isn’t saved until you click “Save Changes”.
Also applies to: 178-180, 286-287, 493-505
🧹 Nitpick comments (10)
internal/services/reannounce/service.go (1)
440-440: Inconsistent debounce window usage in finishJob.Line 440 uses the fixed
s.cfg.DebounceWindowto determine when to delete job records, but enqueue now uses a dynamic window in aggressive mode. This could lead to jobs being retained too long or cleaned up too soon, depending on the aggressive mode interval.Consider using the same dynamic window calculation here for consistency. However, since finishJob doesn't have access to settings, you may need to either:
- Store the debounce window used in the job struct, or
- Pass settings to finishJob, or
- Accept the fixed cleanup window as a reasonable approximation
internal/database/migrations/019_add_tracker_rules.sql (1)
4-30: LGTM! Schema design is solid.The table structure, indexes, and trigger implementation align well with the codebase patterns. The trigger correctly follows the established pattern (AFTER UPDATE with separate UPDATE statement) that doesn't cause recursion with SQLite's default settings.
Based on learnings
Consider adding CHECK constraints for data integrity:
is_default IN (0, 1)to enforce boolean values- Non-negative constraints on limit fields (e.g.,
upload_limit_kib >= 0)- A UNIQUE constraint on
(instance_id, is_default)WHEREis_default = 1if only one default rule per instance is intendedThese constraints would provide additional safety but can be deferred if validation is handled at the application layer.
web/src/types/index.ts (1)
88-118: LGTM! Types align well with the backend.The
TrackerRuleandTrackerRuleInputinterfaces properly map the Go backend types to TypeScript with appropriate camelCase naming and nullable field handling.Consider whether
trackerPatternshould be required (non-optional) inTrackerRuleInputfor creation operations. Currently it's optional, which may be intentional for update operations, but if the backend requires it for creation, you might want to split this into separateTrackerRuleCreateInputandTrackerRuleUpdateInputtypes for better type safety. This is only a suggestion if stricter validation is desired.internal/api/server_test.go (1)
51-56: Tracker-rules endpoints and store wiring look correctThe new tracker-rules routes are properly exempted from the documentation coverage test, and
TrackerRuleStoreis wired into the testDependenciesin the same style as other stores. This should keepTestAllEndpointsDocumentedstable until the OpenAPI spec is updated to include these endpoints.You might consider adding a brief TODO near
undocumentedRoutesfor the tracker-rules entries to remind future work to move them into the spec once the API is finalized.Also applies to: 81-121
web/src/components/instances/preferences/TrackerRulesPanel.tsx (1)
34-47: Tracker rules CRUD, validation, and summary implementation look robustThe panel wires tracker rules to the backend cleanly: rules are scoped by
instanceId, sorted deterministically, and mutations (create/update/delete/reorder/apply) all invalidate the appropriate query. Validation aroundnameand “default vs. at least one tracker” is sound, andparseTrackerDomains’ fallback totrackerPatternkeeps existing rules editable even if domains weren’t previously stored.If you want to refine UX further later, two ideas:
- Add a small toast on successful reordering, or optimistic reordering with rollback on error, so users get feedback when moving rules.
- Optionally guard against deleting the last
isDefaultrule, depending on how strictly the backend enforces a single default.Also applies to: 55-119, 126-177, 179-417, 420-508
internal/api/handlers/tracker_rules.go (1)
210-225: Clarify ApplyNow behavior when the tracker-rule service is not configured
ApplyNowreturns202with"status": "applied"even whenh.service == nil, meaning nothing is actually applied. If a nil service can occur outside of tests, consider either:
- Returning a 503/501 when
h.serviceis nil, or- Logging a warning so it’s visible in diagnostics.
If a nil service is strictly a test-only condition, this is fine as-is.
web/src/lib/api.ts (1)
54-55: Wire tracker-rule API methods with explicit generics for type safetyThe endpoints and payloads look correct and consistent with the backend. To keep type-checking as strong as possible (and consistent with the rest of this file), consider specifying the generic type parameter when calling
this.request:async listTrackerRules(instanceId: number): Promise<TrackerRule[]> { - return this.request(`/instances/${instanceId}/tracker-rules`) + return this.request<TrackerRule[]>(`/instances/${instanceId}/tracker-rules`) } async createTrackerRule(instanceId: number, payload: TrackerRuleInput): Promise<TrackerRule> { - return this.request(`/instances/${instanceId}/tracker-rules`, { + return this.request<TrackerRule>(`/instances/${instanceId}/tracker-rules`, { method: "POST", body: JSON.stringify(payload), }) } async updateTrackerRule(instanceId: number, ruleId: number, payload: TrackerRuleInput): Promise<TrackerRule> { - return this.request(`/instances/${instanceId}/tracker-rules/${ruleId}`, { + return this.request<TrackerRule>(`/instances/${instanceId}/tracker-rules/${ruleId}`, { method: "PUT", body: JSON.stringify(payload), }) }The
delete,reorder, andapplymethods are fine returningPromise<void>as-is.Also applies to: 1222-1257
internal/services/trackerrules/service.go (1)
111-114: Guard ApplyOnceForInstance against misconfigured service dependencies
ApplyOnceForInstancecallsapplyForInstancewithout the defensive nil-checks thatapplyAllhas (forsyncManager,ruleStore,instanceStore). If a service were ever constructed with missing dependencies, a manual apply could panic even though the periodic loop safely does nothing.You could mirror the guard from
applyAll:func (s *Service) ApplyOnceForInstance(ctx context.Context, instanceID int) error { - return s.applyForInstance(ctx, instanceID) + if s == nil || s.syncManager == nil || s.ruleStore == nil { + return nil + } + return s.applyForInstance(ctx, instanceID) }Adjust the behavior (e.g., return an error) to whatever you want callers/handlers to surface.
internal/models/tracker_rule.go (2)
82-147: Consider checking rows.Err() after iterating in ListByInstance
ListByInstancecorrectly scans rows and buildsTrackerRulevalues, but it doesn’t checkrows.Err()after the loop. Adding that check will surface any deferred driver errors:for rows.Next() { // scan, map to rule... } - - return rules, nil + if err := rows.Err(); err != nil { + return nil, err + } + return rules, nil
215-255: Make default-clearing and insert transactional in CreateIn
Create,clearDefaultand theINSERTrun as separate statements on the basedbconnection, whileUpdateuses a transaction andclearDefaultTx. Under concurrent writes for the same instance, this could briefly allow two “default” rules:
- Goroutine A calls
clearDefault, then gets preempted.- Goroutine B calls
clearDefaultandINSERTits default rule.- Goroutine A resumes and
INSERTs another default rule.If you want a strong invariant of at most one default per instance, consider wrapping
clearDefaultandINSERTin a single transaction, similar toUpdate:func (s *TrackerRuleStore) Create(ctx context.Context, rule *TrackerRule) (*TrackerRule, error) { // nil-check, normalize pattern, compute sortOrder... - if rule.IsDefault { - if err := s.clearDefault(ctx, rule.InstanceID); err != nil { - return nil, err - } - } - - res, err := s.db.ExecContext(ctx, ` + tx, err := s.db.BeginTx(ctx, nil) + if err != nil { + return nil, err + } + defer tx.Rollback() + + if rule.IsDefault { + if err := s.clearDefaultTx(ctx, tx, rule.InstanceID); err != nil { + return nil, err + } + } + + res, err := tx.ExecContext(ctx, ` INSERT INTO tracker_rules ... - `, /* args... */) + `, /* args... */) if err != nil { - return nil, err + return nil, err } - + if err := tx.Commit(); err != nil { + return nil, err + }(Then keep using
Getafterwards as you already do.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
cmd/qui/main.go(5 hunks)internal/api/handlers/tracker_rules.go(1 hunks)internal/api/server.go(6 hunks)internal/api/server_test.go(2 hunks)internal/database/migrations/019_add_tracker_rules.sql(1 hunks)internal/models/tracker_rule.go(1 hunks)internal/services/reannounce/service.go(1 hunks)internal/services/trackerrules/service.go(1 hunks)web/package.json(1 hunks)web/src/components/instances/preferences/InstancePreferencesDialog.tsx(2 hunks)web/src/components/instances/preferences/TrackerReannounceForm.tsx(6 hunks)web/src/components/instances/preferences/TrackerRulesPanel.tsx(1 hunks)web/src/components/layout/Header.tsx(2 hunks)web/src/components/layout/MobileFooterNav.tsx(2 hunks)web/src/components/layout/Sidebar.tsx(2 hunks)web/src/components/torrents/TorrentTableOptimized.tsx(3 hunks)web/src/lib/api.ts(2 hunks)web/src/pages/Services.tsx(1 hunks)web/src/routeTree.gen.ts(11 hunks)web/src/routes/_authenticated/services.tsx(1 hunks)web/src/types/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/019_add_tracker_rules.sql
🧬 Code graph analysis (13)
web/src/types/index.ts (1)
internal/models/tracker_rule.go (1)
TrackerRule(16-32)
internal/services/reannounce/service.go (1)
internal/models/instance_reannounce.go (1)
DefaultInstanceReannounceSettings(53-69)
web/src/components/instances/preferences/TrackerRulesPanel.tsx (5)
web/src/types/index.ts (2)
TrackerRuleInput(106-118)TrackerRule(88-104)internal/models/tracker_rule.go (1)
TrackerRule(16-32)web/src/hooks/useInstanceTrackers.ts (1)
useInstanceTrackers(18-27)web/src/components/ui/multi-select.tsx (2)
Option(9-12)MultiSelect(25-161)web/src/lib/api.ts (1)
api(1613-1613)
web/src/pages/Services.tsx (3)
web/src/hooks/useInstances.ts (1)
useInstances(10-185)web/src/components/instances/preferences/TrackerRulesPanel.tsx (1)
TrackerRulesPanel(49-418)web/src/components/instances/preferences/TrackerReannounceForm.tsx (1)
TrackerReannounceForm(54-577)
web/src/components/instances/preferences/TrackerReannounceForm.tsx (2)
web/src/types/index.ts (1)
InstanceReannounceSettings(42-55)internal/models/instance_reannounce.go (1)
InstanceReannounceSettings(25-40)
web/src/routes/_authenticated/services.tsx (1)
web/src/pages/Services.tsx (1)
Services(18-111)
internal/services/trackerrules/service.go (4)
internal/models/instance.go (1)
InstanceStore(127-130)internal/models/tracker_rule.go (2)
TrackerRuleStore(34-36)TrackerRule(16-32)internal/qbittorrent/sync_manager.go (1)
SyncManager(136-140)web/src/types/index.ts (3)
Torrent(204-261)TrackerRule(88-104)Category(342-345)
web/src/lib/api.ts (2)
internal/models/tracker_rule.go (1)
TrackerRule(16-32)web/src/types/index.ts (2)
TrackerRule(88-104)TrackerRuleInput(106-118)
cmd/qui/main.go (2)
internal/models/tracker_rule.go (2)
NewTrackerRuleStore(38-40)TrackerRuleStore(34-36)internal/services/trackerrules/service.go (2)
NewService(50-67)DefaultConfig(30-36)
internal/api/server_test.go (1)
internal/models/tracker_rule.go (2)
TrackerRuleStore(34-36)NewTrackerRuleStore(38-40)
internal/api/handlers/tracker_rules.go (3)
internal/models/tracker_rule.go (2)
TrackerRuleStore(34-36)TrackerRule(16-32)internal/services/trackerrules/service.go (1)
Service(39-48)internal/api/handlers/helpers.go (2)
RespondError(34-38)RespondJSON(22-31)
internal/api/server.go (3)
internal/models/tracker_rule.go (1)
TrackerRuleStore(34-36)internal/services/trackerrules/service.go (1)
Service(39-48)internal/api/handlers/tracker_rules.go (1)
NewTrackerRuleHandler(26-31)
internal/models/tracker_rule.go (2)
web/src/types/index.ts (1)
TrackerRule(88-104)internal/dbinterface/querier.go (2)
Querier(28-33)TxQuerier(17-23)
⏰ 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 (19)
internal/services/reannounce/service.go (1)
352-361: LGTM! Dynamic debounce window correctly applied.The debounce check correctly uses the dynamic window and provides contextual reasons for skipped reannounces based on the mode.
web/src/components/layout/MobileFooterNav.tsx (1)
57-57: LGTM!The addition of the Wrench icon and the Services menu item is clean and follows the established pattern. The placement in the dropdown menu (after Cross-Seed, before Instance Backups) makes sense for the navigation hierarchy.
Also applies to: 349-357
web/src/components/layout/Header.tsx (1)
38-38: LGTM!The Services menu item addition is consistent with the existing navigation pattern and properly positioned in the dropdown menu hierarchy.
Also applies to: 508-516
web/package.json (1)
71-71: LGTM!The addition of
@tanstack/router-clias a dev dependency is appropriate for route generation tooling, and the version matches the@tanstack/react-routerdependency for consistency.web/src/components/layout/Sidebar.tsx (1)
27-28: LGTM!The Services navigation item is properly integrated with consistent icon usage and positioning that matches the other navigation components (Header, MobileFooterNav).
Also applies to: 55-59
web/src/components/instances/preferences/InstancePreferencesDialog.tsx (1)
8-8: LGTM!The removal of the reannounce tab and the corresponding grid column adjustment (from 8 to 7 columns) is correct. This aligns with moving the Tracker Reannounce functionality to the new Services page.
Also applies to: 51-51
web/src/routes/_authenticated/services.tsx (1)
1-11: LGTM!The route file follows the standard TanStack Router pattern and correctly wires the Services page component to the authenticated services path.
internal/api/server.go (1)
36-37: Tracker rules service/store wiring and routes are consistent and well-scopedThe tracker rules store/service are added to
ServerandDependenciesin line with existing services, and the/instances/{instanceID}/tracker-rulesroute group cleanly exposes list/create/update/delete/reorder/apply operations under the authenticated, per-instance namespace. This matches the handler constructor signature and the tests’ undocumented route entries.Also applies to: 67-69, 71-95, 97-128, 263-418
cmd/qui/main.go (1)
41-42: Tracker rules integration into main service lifecycle looks solidThe tracker rules store/service are initialized from the shared DB and
syncManager, started in their own cancellable context, and passed intoapi.NewServeralongside other services. This mirrors the existing reannounce/cross-seed wiring and should behave correctly across startup and graceful shutdown.Also applies to: 477-478, 537-538, 551-554, 616-640
web/src/routeTree.gen.ts (1)
20-21: Generated route tree correctly exposes/servicesunder the authenticated layoutThe additions for
AuthenticatedServicesRoute(imports, route update call, FileRoutes* mappings, andAuthenticatedRouteChildren) are consistent with the existing authenticated routes. Given this file is generated, no manual changes are needed.Also applies to: 50-55, 93-135, 147-176, 218-231, 301-319
web/src/components/torrents/TorrentTableOptimized.tsx (1)
103-104: Reannounce status icon navigation to/servicesis wired correctlyThe new
useNavigateusage cleanly routes users to/serviceswith the currentinstanceIdin the search params, which matches how the Services page derives its selection. Stopping propagation in the click handler avoids interfering with table selection, and the icon remains gated oninstance?.reannounceSettings?.enabled, so behavior stays consistent with the previous indicator semantics.Also applies to: 876-882, 2794-2806
internal/api/handlers/tracker_rules.go (3)
47-71: Payload-to-model mapping and normalization look goodCentralizing trimming, domain normalization, and optional sortOrder in
TrackerRulePayload.toModelis clean and keeps handler logic thin. This also aligns well with theTrackerRulemodel fields.
102-110: Validation correctly enforces name and tracker/default presenceThe
Namecheck and “at least one tracker or default” guard look sound and should prevent creation/update of unusable rules.Also applies to: 141-149
237-263: String cleaning and domain normalization helpers are solid
cleanStringPtrandnormalizeTrackerDomainsnicely avoid storing empty/whitespace values and ensure deduplicated domain lists, which will keep persisted rule data tidy.internal/services/trackerrules/service.go (3)
69-88: Service startup and loop structure look appropriateThe Start/loop pattern with a ticker, context cancellation, and dependency nil-checks in
applyAllis straightforward and should behave well in long-running processes.
325-355: Tracker-domain extraction and tag matching are robustUsing
ExtractDomainFromURLplus a sanitized fallback, set-aggregation, and sorted output gives predictable domain lists to match against. The tag matching helper is also straightforward and case-insensitive, which is what users will expect.Also applies to: 373-383
172-183: Rewritten Review CommentThis review comment contains an incorrect analysis and should be dismissed. The unit conversions are actually correct.
The code properly handles unit consistency:
rule.UploadLimitKiB(KiB) is multiplied by 1024 to createdesired(bytes) for comparison withtorrent.UpLimit✓- The KiB value is correctly batched and passed to
SetTorrentUploadLimit(limit)SetTorrentUploadLimithas a parameter explicitly namedlimitKBsand internally converts to bytes withlimitBytes := limitKBs * 1024(line 3691)Passing
desired(bytes) instead would be incorrect—it would cause the value to be multiplied by 1024 twice. The current implementation correctly passes KiB values to an API that expects KiB input.Likely an incorrect or invalid review comment.
internal/models/tracker_rule.go (2)
16-32: TrackerRule model matches API/web shape wellThe model fields and JSON tags line up with the frontend
TrackerRuletype, including optional limits and timestamps, which should keep serialization round-trips simple.
305-319: Reorder implementation is simple and instance-safeReordering uses a transaction and scopes the
UPDATEby bothidandinstance_id, which is the right safety guard for cross-instance IDs. As long as callers pass the full ordered ID list per instance, this should behave predictably.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/services/reannounce/service_test.go (2)
372-372: Consider renaming the test to reflect the updated behavior.The test name
TestServiceEnqueue_AggressiveModeSkipsDebounceis misleading after the changes. Aggressive mode no longer skips debounce entirely—it now respects a cooldown period based onReannounceIntervalSeconds. Consider renaming to something likeTestServiceEnqueue_AggressiveModeUsesRetryIntervalForCooldownto accurately describe the behavior being tested.
396-404: Test logic correctly validates the new aggressive mode cooldown behavior.The updated test properly verifies that aggressive mode now respects a per-instance cooldown based on
ReannounceIntervalSeconds. The test flow is clear:
- At 5 seconds after completion: job correctly debounces (5 < 7 second interval)
- At 15 seconds: cooldown expires, new job starts (15 > 7)
Optional: Consider adding a test case for the default behavior when
ReannounceIntervalSecondsis 0 or unset. The AI summary mentions "fallback to defaults if missing," which would be good to verify explicitly:// Test case for default retry interval when not configured svc.settingsCache.Replace(&models.InstanceReannounceSettings{ InstanceID: 1, Aggressive: true, Enabled: true, // ReannounceIntervalSeconds not set or 0 }) // Verify expected default cooldown behavior
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/services/reannounce/service_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/services/reannounce/service_test.go (2)
web/src/types/index.ts (1)
InstanceReannounceSettings(42-55)internal/models/instance_reannounce.go (1)
InstanceReannounceSettings(25-40)
⏰ 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). (7)
- GitHub Check: Build Docker images (linux/arm/v6)
- GitHub Check: Build Docker images (linux/arm/v7)
- GitHub Check: Build Docker images (linux/arm64)
- GitHub Check: Build Docker images (linux/amd64/v3)
- GitHub Check: Build Docker images (linux/amd64/v2)
- GitHub Check: Build Docker images (linux/amd64)
- GitHub Check: Build distribution binaries
…instance selection
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web/src/pages/Services.tsx (1)
23-38: Align selection, URL param, and “active instance” messaging
selectedInstanceIdis derived from allinstances(including inactive), the<Select>lists all instances, butselectedInstanceis looked up only inactiveInstances. This means:
- Picking an inactive instance keeps the select and
instanceIdURL param pointing at that inactive ID.selectedInstanceisundefined, so the page shows “Select an active instance to configure services” and renders no panels.This reproduces the earlier concern about the URL/UI getting out of sync for inactive instances.
To make behavior predictable, consider one of:
- Only allow active instances to be selected (simpler, matches copy):
- const activeInstances = useMemo( - () => (instances ?? []).filter((instance) => instance.isActive), - [instances] - ) + const activeInstances = useMemo( + () => (instances ?? []).filter((instance) => instance.isActive), + [instances] + ) const selectedInstanceId = useMemo(() => { const fromSearch = search.instanceId ? Number(search.instanceId) : undefined - const allInstances = instances ?? [] - if (fromSearch && allInstances.some((inst) => inst.id === fromSearch)) { + const allActive = activeInstances + if (fromSearch && allActive.some((inst) => inst.id === fromSearch)) { return fromSearch } - if (allInstances.length > 0) { - return allInstances[0]?.id + if (allActive.length > 0) { + return allActive[0]?.id } return undefined - }, [instances, search.instanceId]) + }, [activeInstances, search.instanceId])- {instances && instances.length > 0 && ( + {activeInstances.length > 0 && ( <Select value={selectedInstanceId ? String(selectedInstanceId) : undefined} onValueChange={handleInstanceChange} > @@ - <SelectContent> - {(instances ?? []).map((instance) => ( + <SelectContent> + {activeInstances.map((instance) => (
- Or derive
selectedInstancefrom all instances and adjust the messaging to reflect that services may be unavailable for inactive ones, ensuring the URL param, selected option, and rendered instance always refer to the same object.Also applies to: 51-51, 63-88, 97-101
🧹 Nitpick comments (1)
internal/services/trackerrules/service.go (1)
111-115: Clarify/adjust debounce behavior for manual “Apply once” and consider nil‑safety
ApplyOnceForInstancecallsapplyForInstancedirectly, andapplyForInstanceuses the sameSkipWithindebounce window as the periodic loop:if ok && now.Sub(ts) < s.cfg.SkipWithin { continue } ... s.mu.Lock() instLastApplied[torrent.Hash] = now s.mu.Unlock()This means a user who just edited rules and presses “Apply now” may see no changes for torrents that were processed in the last
SkipWithin(default 2 minutes), even though they explicitly requested an immediate apply. The same applies if they’ve manually changed limits in qBittorrent and expect this to override right away.Two possible improvements:
- Bypass debounce for manual runs
TreatApplyOnceForInstanceas an override path:-func (s *Service) ApplyOnceForInstance(ctx context.Context, instanceID int) error { - return s.applyForInstance(ctx, instanceID) -} +func (s *Service) ApplyOnceForInstance(ctx context.Context, instanceID int) error { + if s == nil { + return nil + } + // Option A: bypass debounce window for explicit/manual runs + return s.applyForInstanceWithOptions(ctx, instanceID, true) +}with an internal helper that optionally skips the
SkipWithincheck (or resetsinstLastAppliedbefore the loop). That keeps the background loop gentle on qBittorrent while ensuring explicit user actions are honored immediately.
- Keep debounce but document it and tighten safety
If you prefer to keep debouncing even for manual triggers, it’s still worth:
- Adding a brief comment where
ApplyOnceForInstanceis used or on the API route explaining that “Apply now” is subject to the same debounce as the background worker.- Adding a small nil-guard for consistency with
Start:func (s *Service) ApplyOnceForInstance(ctx context.Context, instanceID int) error { - return s.applyForInstance(ctx, instanceID) + if s == nil { + return nil + } + return s.applyForInstance(ctx, instanceID) }This isn’t a correctness bug, but adjusting or documenting the behavior will avoid confusion when users click “Apply now” and see no effect due to the debounce window.
Also applies to: 136-167, 199-204
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/api/handlers/tracker_rules.go(1 hunks)internal/services/reannounce/service.go(1 hunks)internal/services/trackerrules/service.go(1 hunks)web/src/pages/Services.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/api/handlers/tracker_rules.go
🧰 Additional context used
🧬 Code graph analysis (2)
web/src/pages/Services.tsx (3)
web/src/hooks/useInstances.ts (1)
useInstances(10-185)web/src/components/instances/preferences/TrackerRulesPanel.tsx (1)
TrackerRulesPanel(49-418)web/src/components/instances/preferences/TrackerReannounceForm.tsx (1)
TrackerReannounceForm(54-577)
internal/services/trackerrules/service.go (3)
internal/models/tracker_rule.go (1)
TrackerRule(16-32)internal/qbittorrent/sync_manager.go (1)
SyncManager(136-140)web/src/types/index.ts (3)
Torrent(204-261)TrackerRule(88-104)Category(342-345)
⏰ 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 (3)
internal/services/reannounce/service.go (1)
341-358: LGTM! Dynamic debounce window correctly implemented.The logic correctly computes a per-instance debounce window based on aggressive mode settings and applies it consistently in the debounce check. The contextual skip reasons ("cooldown window" vs "retry interval window") improve observability.
The previous review concern about an unreachable
settings == nilcheck has been addressed—the current code safely accessessettings.ReannounceIntervalSecondsonly whenisAggressiveis true, which guaranteessettings != nil.web/src/pages/Services.tsx (1)
18-22: Services page wiring and layout look solidInstance fetching, URL search integration, and conditional rendering of
TrackerRulesPanelandTrackerReannounceFormare cohesive and match the new Services route usage. Once the instance-selection behavior is tightened up, this page should be in good shape.Also applies to: 53-61, 103-110
internal/services/trackerrules/service.go (1)
23-67: Tracker‑rules service logic, batching, and matching look solid
- Config defaults, dependency guards in
applyAll, and per-instancelastAppliedtracking withsync.RWMutexare implemented carefully and avoid data races.- Batch construction in
applyForInstancerespects KiB vs bytes, only enqueues changes where limits/ratio/time differ, and useslimitHashBatchto cap batch sizes without mutating the original slices.selectRule+matchesTracker+collectTrackerDomainscorrectly leverageqbt.Torrent.TrackersandTorrentTracker.Urlfields fromgit.832008.xyz/autobrr/go-qbittorrentfor domain-based matching, with a clean fallback toIsDefaultrules when no specific tracker match is found.- Tag/category filters and case-insensitive comparisons (
torrentHasTag,EqualFold) align with how qBittorrent exposes string fields and should be robust across trackers.No blocking issues spotted in the core rule‑application path.
Also applies to: 69-110, 239-380
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/services/reannounce/REANNOUNCE.md (2)
41-45: Update Activity Log navigation to reflect Services page move.The Activity Log section in REANNOUNCE.md contains outdated navigation instructions. The Tracker Reannounce feature, including Activity Log, has moved from Settings > Instances to the Services page. Update lines 41-45 to reflect the new location:
Current (incorrect):
1. Go to the **Tracker Reannounce** tab. 2. Click **Activity Log**.Corrected:
1. Go to the **Services** page. 2. Select your instance from the dropdown. 3. In the **Tracker Reannounce** section, click the **Activity Log** tab.
11-14: Update Quick Start and Activity Log navigation to reflect Services page move.The Tracker Reannounce feature has been moved to the Services page, but the documentation at lines 11-14 and 43-44 still references the old location (Settings > Instances). Users will not find Tracker Reannounce there.
Update lines 11-14 to:
1. Go to **Services** in the main navigation. 2. Select an instance from the dropdown. 3. In the **Tracker Reannounce** section, toggle **Enabled** to turn it on. 4. Click **Save Changes**.Update lines 43-44 to:
1. Go to **Services** and select your instance. 2. Click the **Activity Log** tab in the Tracker Reannounce section.
🧹 Nitpick comments (2)
web/src/components/instances/preferences/TrackerReannounceForm.tsx (2)
124-155: Centralized persistence looks good; consider syncing sanitized state and handling rollback on errorConsolidating update logic into
persistSettingsis a solid improvement and keeps submit/toggle flows consistent. One follow‑up you might consider:
- After
sanitizeSettings(nextSettings), the UIsettingsstate is not updated with the sanitized version. If sanitize trims/clamps values, the DB and UI can temporarily diverge until the component remounts or data is re-fetched.- When called from
handleToggleEnabled(false), the UI is already updated to “disabled” before the mutation completes. If the request fails, the UI still shows disabled while the server is not, andactivityQueryalso disables polling.If you want tighter UX, you could set
settingstosanitizedand optionally revert the relevant bits inonError.
162-169: Disable persists immediately; consider what should happen on failureThe toggle flow (local state update, then persisting only when disabling) matches the described UX: turning off monitoring is immediate, turning it on requires Save.
One caveat: if the disable call fails, the UI still shows monitoring disabled and
activityEnabledbecomes false while the server may still be running reannounces. You might want to either:
- revert
enabledtotrueinonError, or- surface a stronger error hint telling the user the disable didn’t apply.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)internal/services/reannounce/REANNOUNCE.md(2 hunks)web/src/components/instances/preferences/TrackerReannounceForm.tsx(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-06T12:11:04.963Z
Learnt from: Audionut
Repo: autobrr/qui PR: 553
File: internal/services/crossseed/service.go:1045-1082
Timestamp: 2025-11-06T12:11:04.963Z
Learning: The autobrr/qui project uses a custom go-qbittorrent client library (github.com/autobrr/go-qbittorrent) that supports both "paused" and "stopped" parameters when adding torrents via the options map. Both parameters should be set together when controlling torrent start state, as seen in internal/services/crossseed/service.go and throughout the codebase.
Applied to files:
README.mdinternal/services/reannounce/REANNOUNCE.md
⏰ 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 (8)
README.md (1)
38-39: Documentation updates accurately reflect the new features.Both the Tracker Reannounce description update and the new Tracker Rules feature entry are clear, user-friendly, and consistent with the PR objectives. The descriptions concisely explain what each feature does without unnecessary jargon.
web/src/components/instances/preferences/TrackerReannounceForm.tsx (7)
157-161: Form submit wiring is straightforward and correctThe submit handler cleanly prevents default and delegates to
persistSettings(settings), matching the new centralized flow and theButton type="submit"usage.
205-239: Header wiring and enable toggle integration look solidThe Card/header restructuring, tooltip for “Automatic Tracker Reannounce”, and the switch wiring (
checked,onCheckedChange,disabled={isUpdating}) all look consistent and should prevent double-toggles while an update is in flight.
271-308: Quick Retry copy/readability improvements look goodRenaming the section to “Quick Retry” and updating the tooltip/description to spell out the cooldown behavior against the default 2 minutes clarifies usage without changing behavior (
settings.aggressivestill drives it). No issues from a code perspective.
357-360: Tabs trigger layout changes are purely stylisticOnly whitespace/formatting changed here; behavior (
value,disabled, classes) remains the same.
388-391: Same for tags include toggle – no functional changeThe tags include
TabsTriggeredits are layout-only; runtime behavior is unchanged.
419-422: Trackers include toggle edits are also cosmeticTracker domain
TabsTriggerchanges are formatting-only, with identical props and behavior.
508-515: Activity empty state visual update is safeThe dashed-border placeholder and padding changes are visual only; logic for when it appears and what it says is unchanged.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
web/src/components/layout/MobileFooterNav.tsx (1)
392-400: Services dropdown entry looks good; optionally consider instance scopingThe new Services item follows the existing dropdown pattern (
DropdownMenuItem+Link, icon + label) and correctly targets/services. If the Services page can accept an instance identifier (e.g., via route params or search) and you want to minimize extra taps on mobile, you could optionally pass the currently active instance fromcurrentInstanceIdhere; otherwise this is fine as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
web/src/components/layout/Header.tsx(2 hunks)web/src/components/layout/MobileFooterNav.tsx(2 hunks)web/src/components/layout/Sidebar.tsx(2 hunks)web/src/components/torrents/TorrentTableOptimized.tsx(3 hunks)web/src/types/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- web/src/components/layout/Header.tsx
- web/src/types/index.ts
- web/src/components/layout/Sidebar.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
web/src/components/layout/MobileFooterNav.tsx (1)
web/src/components/ui/dropdown-menu.tsx (1)
DropdownMenuItem(251-251)
⏰ 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 (3)
web/src/components/layout/MobileFooterNav.tsx (1)
62-62: Wrench icon import is correctly wired to the new menu itemThe added
Wrenchimport is used in the Services dropdown entry below and fits the existing lucide-react icon pattern; no issues here.web/src/components/torrents/TorrentTableOptimized.tsx (2)
103-103: LGTM! Router hooks properly imported and initialized.The
useNavigateanduseSearchhooks from TanStack Router are correctly imported and initialized following React's Rules of Hooks.Also applies to: 879-879
2804-2807: Verification complete—navigation implementation is correct.The
/servicesroute is properly defined inweb/src/routes/_authenticated/services.tsx, and the search parameter schema atweb/src/pages/Services.tsx:14-16correctly declaresinstanceIdas an optional string:type ServicesSearch = { instanceId?: string }. The type conversion flow is sound: theinstanceIdnumber is converted to a string for the URL search parameter and then converted back to a number in the Services page component (line 29:Number(search.instanceId)). The navigation call is valid and type-safe.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
web/src/components/instances/preferences/TrackerReannounceForm.tsx(9 hunks)web/src/components/ui/multi-select.tsx(2 hunks)web/src/hooks/useInstances.ts(1 hunks)web/src/pages/CrossSeedPage.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
web/src/hooks/useInstances.ts (2)
web/src/types/index.ts (1)
InstanceResponse(120-125)internal/api/handlers/instances.go (1)
InstanceResponse(352-366)
web/src/components/instances/preferences/TrackerReannounceForm.tsx (3)
web/src/components/torrents/CategoryTree.tsx (2)
buildCategoryTree(50-105)CategoryNode(19-27)web/src/components/ui/multi-select.tsx (1)
Option(9-13)web/src/types/index.ts (1)
InstanceReannounceSettings(42-55)
web/src/pages/CrossSeedPage.tsx (1)
web/src/components/torrents/CategoryTree.tsx (2)
buildCategoryTree(50-105)CategoryNode(19-27)
⏰ 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 (12)
web/src/components/ui/multi-select.tsx (2)
9-13: LGTM! Optional property enables hierarchical rendering.The optional
levelproperty is a clean addition that maintains backward compatibility while enabling indented option trees.
156-161: LGTM! Indentation logic correctly implemented.The conditional padding calculation (level × 12px) provides clear visual hierarchy for nested options.
web/src/pages/CrossSeedPage.tsx (2)
6-6: LGTM! Import statement is correct.Consistent with similar usage in TrackerReannounceForm.tsx.
683-711: LGTM! Tree-based category handling correctly implemented.The refactor correctly:
- Builds a hierarchical category tree
- Flattens it with level information for indentation
- Preserves manually typed categories as extras with level 0
The
visitNodesrecursive traversal and extras filtering logic are both sound.web/src/components/instances/preferences/TrackerReannounceForm.tsx (8)
17-17: LGTM! Import statement is correct.Consistent with the hierarchical category handling pattern used in CrossSeedPage.tsx.
89-109: LGTM! Category tree building correctly implemented.The hierarchical category structure is correctly built and flattened with level information. Note that unlike CrossSeedPage.tsx, this implementation doesn't append manually typed extras—this appears intentional since the MultiSelect component has
creatableenabled andappendUniqueValuehandles custom entries.
137-168: Good refactoring! Extracted submission logic into reusable helper.The
persistSettingshelper reduces code duplication and enables customizable success messages. The sanitization and error handling are appropriate.
170-173: LGTM! Simple and correct submit handler.Properly delegates to
persistSettingswith appropriate default behavior.
175-182: Good UX enhancement! Immediate persistence when disabling.The auto-save behavior when disabling monitoring provides good user feedback while allowing users to configure settings before enabling. This is a sensible UX pattern.
218-250: LGTM! Improved header layout and UX.The enhancements provide:
- Better responsive design with flex-row on larger screens
- Helpful tooltip explaining the automatic reannounce feature
- Clearer visual styling for the enable/disable toggle
- Proper disabled state during updates
284-284: LGTM! Terminology change improves clarity.The rename from "Aggressive Mode" to "Quick Retry" is more user-friendly and accurately describes the behavior. The change is applied consistently across labels, tooltips, and descriptions while maintaining backend compatibility with the
aggressiveproperty.Also applies to: 303-318
521-521: LGTM! Dashed border improves empty state visual.The dashed border styling better conveys the placeholder nature of the empty activity state.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
README.md(1 hunks)internal/api/handlers/tracker_rules.go(1 hunks)internal/database/migrations/019_add_tracker_rules.sql(1 hunks)internal/models/tracker_rule.go(1 hunks)internal/services/trackerrules/TRACKER_RULES.md(1 hunks)internal/services/trackerrules/service.go(1 hunks)web/src/components/instances/preferences/TrackerRulesPanel.tsx(1 hunks)web/src/hooks/useInstances.ts(1 hunks)web/src/types/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- web/src/hooks/useInstances.ts
- README.md
- web/src/types/index.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-25T11:39:54.717Z
Learnt from: s0up4200
Repo: autobrr/qui PR: 637
File: web/src/pages/Dashboard.tsx:805-831
Timestamp: 2025-11-25T11:39:54.717Z
Learning: In web/src/pages/Dashboard.tsx, the TrackerIconImage component intentionally receives displayDomain (incognito-mapped name) instead of the real domain in incognito mode. This causes icon lookups to fail and show only fallback letters, which is desired behavior for privacy - hiding both tracker names and icons when incognito mode is enabled.
Applied to files:
web/src/components/instances/preferences/TrackerRulesPanel.tsx
📚 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/019_add_tracker_rules.sql
🧬 Code graph analysis (2)
internal/api/handlers/tracker_rules.go (3)
internal/models/tracker_rule.go (2)
TrackerRuleStore(34-36)TrackerRule(16-32)internal/services/trackerrules/service.go (1)
Service(39-48)internal/api/handlers/helpers.go (2)
RespondError(34-38)RespondJSON(22-31)
internal/models/tracker_rule.go (1)
internal/dbinterface/querier.go (1)
Querier(30-35)
🪛 markdownlint-cli2 (0.18.1)
internal/services/trackerrules/TRACKER_RULES.md
51-51: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/models/tracker_rule.go (1)
291-305: Silent no-op for non-matching IDs in Reorder.If
orderedIDscontains IDs that don't exist or belong to a different instance, those UPDATEs will affect zero rows without any error. This is defensive but could mask issues where the frontend sends stale IDs.Consider whether you want to verify all IDs were successfully updated:
func (s *TrackerRuleStore) Reorder(ctx context.Context, instanceID int, orderedIDs []int) error { tx, err := s.db.BeginTx(ctx, nil) if err != nil { return err } defer tx.Rollback() for idx, id := range orderedIDs { - if _, err := tx.ExecContext(ctx, `UPDATE tracker_rules SET sort_order = ? WHERE id = ? AND instance_id = ?`, idx+1, id, instanceID); err != nil { + res, err := tx.ExecContext(ctx, `UPDATE tracker_rules SET sort_order = ? WHERE id = ? AND instance_id = ?`, idx+1, id, instanceID) + if err != nil { return err } + if rows, _ := res.RowsAffected(); rows == 0 { + return sql.ErrNoRows // or a custom error indicating invalid rule ID + } } return tx.Commit() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/models/tracker_rule.go(1 hunks)internal/services/trackerrules/TRACKER_RULES.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/services/trackerrules/TRACKER_RULES.md
🧰 Additional context used
🧬 Code graph analysis (1)
internal/models/tracker_rule.go (2)
web/src/types/index.ts (1)
TrackerRule(88-104)internal/dbinterface/querier.go (1)
Querier(30-35)
⏰ 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 (6)
internal/models/tracker_rule.go (6)
16-40: LGTM!The
TrackerRulestruct fields are well-defined with appropriate pointer types for nullable database columns, and the JSON tags align with the frontend TypeScript interface.
42-80: LGTM!The pattern normalization logic correctly handles multiple delimiters, whitespace trimming, and deduplication while preserving insertion order.
149-204: LGTM!The
Getmethod correctly scans nullable columns and derivesTrackerDomains. Its id-only lookup is appropriate since it's called after operations that already verify instance ownership via their WHERE clauses.
215-249: LGTM!The
Createmethod correctly normalizes the tracker pattern, auto-assigns sort order when not specified, and retrieves the created rule. The pattern of treatingsortOrder == 0as "auto-assign" is reasonable.
251-278: RowsAffected check is now in place.The previous review concern about potential cross-instance information leak has been addressed. The method now correctly checks
RowsAffected()and returnssql.ErrNoRowswhen the UPDATE matches zero rows before callingGet.
307-333: LGTM!The nullable helper functions correctly handle pointer-to-value conversion for SQL parameters, and
boolToIntappropriately handles SQLite's lack of native boolean type.
1 participant
Summary by CodeRabbit
New Features
UI / Navigation
Bug Fixes / Behavior
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.