Fix Seeker's filters allowing searching for all types of movies regardless of settings#567
Merged
Conversation
Contributor
Author
|
@sourcery-ai review |
Contributor
Author
|
@greptileai review |
Contributor
Author
|
/review |
Code Review by Qodo
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds extensive Seeker unit tests for Radarr and Sonarr proactive search behaviors and side effects, and removes a redundant exclusion clause from Seeker candidate filtering that depended on both Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideAdjusts Seeker’s Radarr and Sonarr proactive-search filters so that, when cutoff/custom-format filters are disabled, only genuinely missing items are eligible for search (not items that already have files), and adds an extensive test suite covering Radarr/Sonarr filtering, history/cleanup, dry-run behavior, round-robin, replacement queue handling, and SignalR notifications. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new test coverage is very thorough but makes
SeekerTestsextremely long and repetitive; consider extracting common arrange/setup patterns into helper methods or test data builders to keep individual tests focused on behavior and easier to maintain. - Several tests reconfigure
SeekerConfigsandSeekerInstanceConfigsin slightly different ways; introducing factory/helper methods for creating instance configs with specific combinations ofUseCutoff,UseCustomFormatScore, andMonitoredOnlywould reduce boilerplate and help avoid configuration mistakes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new test coverage is very thorough but makes `SeekerTests` extremely long and repetitive; consider extracting common arrange/setup patterns into helper methods or test data builders to keep individual tests focused on behavior and easier to maintain.
- Several tests reconfigure `SeekerConfigs` and `SeekerInstanceConfigs` in slightly different ways; introducing factory/helper methods for creating instance configs with specific combinations of `UseCutoff`, `UseCustomFormatScore`, and `MonitoredOnly` would reduce boilerplate and help avoid configuration mistakes.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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 `@code/backend/Cleanuparr.Infrastructure.Tests/Features/Jobs/SeekerTests.cs`:
- Around line 2068-2156: The test
ExecuteAsync_Radarr_UseCutoffAndUseCustomFormatScore_OrLogic currently can pass
without any search happening; update it to actually assert searches occurred by
(1) invoking sut.ExecuteAsync() twice as intended (call it a second time instead
of recreating context immediately) or keep the RecreateDataContext() but ensure
you call sut.ExecuteAsync() again after recreation, (2) assert that
capturedSearchItems has at least one entry after the first run (e.g.,
capturedSearchItems.Count > 0) and/or at least two entries after the second run
before asserting capturedSearchItems.ShouldNotContain(item => item.Id == 4), and
(3) keep using mockArrClient.SearchItemAsync interception to populate
capturedSearchItems so the assertions validate that a search actually executed;
references: test method
ExecuteAsync_Radarr_UseCutoffAndUseCustomFormatScore_OrLogic,
sut.ExecuteAsync(), capturedSearchItems, mockArrClient.SearchItemAsync, and
_fixture.RecreateDataContext().
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 37de8100-a090-443a-99cb-11a737b945ae
📒 Files selected for processing (2)
code/backend/Cleanuparr.Infrastructure.Tests/Features/Jobs/SeekerTests.cscode/backend/Cleanuparr.Infrastructure/Features/Jobs/Seeker.cs
💤 Files with no reviewable changes (1)
- code/backend/Cleanuparr.Infrastructure/Features/Jobs/Seeker.cs
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR fixes a bug where Seeker's proactive movie search was ignoring the Confidence Score: 5/5Safe to merge — the production fix is correct and well-tested; only remaining finding is a minor test assertion gap. The filter fix is targeted and correct, the Sonarr path was already correctly gated, and the new test suite covers the main regression extensively. The single P2 finding is a test quality issue (weak assertion in one test) with no impact on production behavior. No files require special attention for the production fix; the test assertion gap in SeekerTests.cs is worth addressing but is not blocking. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ExecuteAsync] --> B{SearchEnabled?}
B -- No --> Z[Return]
B -- Yes --> C{ReplacementItem in queue?}
C -- Yes --> D[ProcessReplacementItemAsync\nSearch & remove from queue]
D --> Z
C -- No --> E{ProactiveSearchEnabled?}
E -- No --> Z
E -- Yes --> F[ProcessProactiveSearchAsync\nLoad enabled SeekerInstanceConfigs]
F --> G{UseRoundRobin?}
G -- Yes --> H[Order by oldest LastProcessedAt\nProcess first instance that triggers a search]
G -- No --> I[Process all instances sequentially]
H --> J[ProcessSingleInstanceAsync]
I --> J
J --> K{ActiveDownloadLimit reached?}
K -- Yes --> Z2[Skip instance]
K -- No --> L[ProcessInstanceAsync]
L --> M{InstanceType}
M -- Radarr --> N[ProcessRadarrAsync\nFetch movies + tags + CF scores]
M -- Sonarr --> O[ProcessSonarrAsync\nFetch series + episodes]
N --> P{Filter candidates}
P --> P1["• Status = released\n• Past grace period\n• MonitoredOnly filter\n• SkipTags filter\n• !HasFile\n OR UseCutoff AND QualityCutoffNotMet\n OR UseCustomFormatScore AND CF score below cutoff"]
P1 --> Q{Candidates empty?}
Q -- No --> R[Exclude queued movies\nCycle management\nSelect one via IItemSelector\nSearchItemAsync]
Q -- Yes --> Z2
Reviews (1): Last reviewed commit: "added more tests" | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
code/backend/Cleanuparr.Infrastructure.Tests/Features/Jobs/SeekerTests.cs (1)
2068-2158:⚠️ Potential issue | 🟠 MajorThis still doesn’t prove both OR branches work.
This test is stronger now, but one
ExecuteAsync()call still only proves that either movie2or movie3was eligible. It will still pass if one side of theUseCutoff || UseCustomFormatScorelogic regresses and only the other branch remains searchable. Run twice and assert both2and3are searched, while4never is.Suggested tightening
- // Act - await sut.ExecuteAsync(); + // Act + await sut.ExecuteAsync(); + await sut.ExecuteAsync(); - // Assert — a search happened, and the excluded candidate (both filters met) was never selected - capturedSearchItems.ShouldNotBeEmpty(); - capturedSearchItems.ShouldAllBe(item => item.Id == 2 || item.Id == 3); + // Assert — both OR branches produced an eligible search, and the excluded candidate never did + await mockArrClient.Received(2) + .SearchItemAsync(radarrInstance, Arg.Any<SearchItem>()); + capturedSearchItems.ShouldContain(item => item.Id == 2); + capturedSearchItems.ShouldContain(item => item.Id == 3); capturedSearchItems.ShouldNotContain(item => item.Id == 4);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/backend/Cleanuparr.Infrastructure.Tests/Features/Jobs/SeekerTests.cs` around lines 2068 - 2158, The test only calls sut.ExecuteAsync() once so it doesn't guarantee both OR branches (UseCutoff vs UseCustomFormatScore) are exercised; modify ExecuteAsync_Radarr_UseCutoffAndUseCustomFormatScore_OrLogic to call await sut.ExecuteAsync() a second time (or run the selection flow twice) and then assert that capturedSearchItems contains both Id 2 and Id 3 (and still never contains Id 4), using the existing capturedSearchItems list populated by the mockArrClient.SearchItemAsync to verify both branches ran; keep all other setup (radarrInstance, CustomFormatScoreEntry entries, mockArrClient, ArrClientFactory wiring) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/backend/Cleanuparr.Infrastructure.Tests/Features/Jobs/SeekerTests.cs`:
- Around line 2277-2509: The three Sonarr tests only assert the single captured
SeriesSearchItem and can miss a bug where filtered seasons still get searched
later; update each test
(ExecuteAsync_Sonarr_MissingOnly_ExcludesEpisodesWithFiles,
ExecuteAsync_Sonarr_UseCutoff_ExcludesEpisodesMeetingCutoff,
ExecuteAsync_Sonarr_UseCustomFormatScore_ExcludesEpisodesAboveCutoff) to capture
every SearchItem passed to mockArrClient.SearchItemAsync (e.g., append
ci.ArgAt<SearchItem>(1) cast to SeriesSearchItem into a local list) across the
sut.ExecuteAsync() run(s) and then assert that the list contains the expected
allowed season(s) and does NOT contain the excluded season (e.g., no
SeriesSearchItem with Id == 2); keep using the existing mockArrClient,
SeriesSearchItem type, SearchItemAsync method, CreateSut and ExecuteAsync to
locate where to change the test capture and final assertions.
- Around line 1988-2066: The test currently only asserts that some searches
occurred, which can pass due to movie 1 being missing; update
ExecuteAsync_Radarr_UseCustomFormatScore_ExcludesMoviesAboveCutoff to explicitly
assert the custom-format branch by checking capturedSearchItems contains a
SearchItem for movie with Id == 2 (the below-cutoff CF entry) and still does not
contain Id == 3 (above-cutoff), e.g. assert capturedSearchItems.Any(item =>
item.Id == 2) and capturedSearchItems.All(item => item.Id != 3); use the
existing capturedSearchItems variable to locate and assert the expected entries
so the test verifies CF filtering logic in the SUT's ExecuteAsync path.
---
Duplicate comments:
In `@code/backend/Cleanuparr.Infrastructure.Tests/Features/Jobs/SeekerTests.cs`:
- Around line 2068-2158: The test only calls sut.ExecuteAsync() once so it
doesn't guarantee both OR branches (UseCutoff vs UseCustomFormatScore) are
exercised; modify ExecuteAsync_Radarr_UseCutoffAndUseCustomFormatScore_OrLogic
to call await sut.ExecuteAsync() a second time (or run the selection flow twice)
and then assert that capturedSearchItems contains both Id 2 and Id 3 (and still
never contains Id 4), using the existing capturedSearchItems list populated by
the mockArrClient.SearchItemAsync to verify both branches ran; keep all other
setup (radarrInstance, CustomFormatScoreEntry entries, mockArrClient,
ArrClientFactory wiring) unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d4cbab3-0b90-4e2a-a63c-cdf8452f8bf6
📒 Files selected for processing (1)
code/backend/Cleanuparr.Infrastructure.Tests/Features/Jobs/SeekerTests.cs
12dcf8b to
83e9847
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Tighten Seeker’s proactive search filtering so only items matching the configured criteria are searched, and extend coverage with comprehensive tests around Radarr/Sonarr filters, history, round‑robin behavior, replacement searches, dry runs, and notifications.
Bug Fixes:
Tests:
Summary by CodeRabbit
Bug Fixes
Tests