feat(issues): admin action to mark issue as failed and redownload#2915
feat(issues): admin action to mark issue as failed and redownload#2915babyhuey wants to merge 11 commits intoseerr-team:developfrom
Conversation
📝 WalkthroughWalkthroughAdds an admin-only endpoint to mark grabbed media as failed and trigger Radarr/Sonarr rescans, plus server-side servarr helpers, core redownload logic, route + tests, and frontend UI with confirmation and localization strings. Changes
Sequence DiagramsequenceDiagram
actor Admin
participant Frontend as Frontend
participant Backend as Backend Server
participant DB as Database
participant Servarr as Radarr/Sonarr
Admin->>Frontend: Click "Mark as Failed & Redownload"
Frontend->>Frontend: Show confirmation modal
Admin->>Frontend: Confirm action
Frontend->>Backend: POST /issue/{issueId}/redownload
Backend->>DB: Load Issue + Media
DB-->>Backend: Issue data
alt Movie
Backend->>Servarr: GET /history?movieId=...
Servarr-->>Backend: History records
Backend->>Servarr: POST /history/failed/{historyId}
Servarr-->>Backend: OK
Backend->>Servarr: GET /movieFile/{movieFileId}
Servarr-->>Backend: File details
Backend->>Servarr: DELETE /movieFile/{movieFileId}
Servarr-->>Backend: OK
Backend->>Servarr: POST /command (searchMovie)
Servarr-->>Backend: Command queued
else TV Series
Backend->>Servarr: GET /episode?seriesId=...
Servarr-->>Backend: Episodes
Backend->>Servarr: GET /history?seriesId=...
Servarr-->>Backend: History records
Backend->>Servarr: POST /history/failed/{historyId}
Servarr-->>Backend: OK
Backend->>Servarr: DELETE /episodeFile/{episodeFileId}
Servarr-->>Backend: OK (x N)
Backend->>Servarr: POST /command (searchEpisodes/searchSeason/searchSeries)
Servarr-->>Backend: Command queued
end
Backend->>DB: Add IssueComment, set Issue.status = RESOLVED, save
DB-->>Backend: Persisted
Backend-->>Frontend: 200 OK + Updated Issue
Frontend->>Frontend: Revalidate data, show success toast
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
babyhuey
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
server/lib/issueRedownload.ts (2)
80-85: Consider isolating the "mark failed → delete file → search" steps so a mid-flow failure doesn’t leave a partially-applied state.The three API calls are executed without per-step error handling. If
markGrabAsFailedsucceeds butdeleteMovieFile/deleteEpisodeFilethrows (e.g., file already removed out-of-band, transient *arr error), the whole function aborts — the release is blocklisted but the file is still present, so the subsequent manual retry will find nograbbedentry to blocklist and the *arr search will still be a no-op because the file remains on disk. A light try/catch around the delete (logging + continuing to the search) would make the flow more tolerant of benign404s from *arr for files that no longer exist.Non-blocking — feel free to defer, but worth considering for the retry experience.
Also applies to: 154-182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/issueRedownload.ts` around lines 80 - 85, The sequence that marks a grab failed then deletes the file then triggers a search can leave the system in a half-applied state if deleteMovieFile/deleteEpisodeFile throws; wrap the delete call(s) (references: deleteMovieFile, deleteEpisodeFile and the surrounding radarr.getMovie usage) in a try/catch that logs the error (including 404/not-found cases) and continues to call radarr.searchMovie so the search is attempted even when the delete fails, and ensure markGrabAsFailed remains unchanged; treat 404-like errors as non-fatal and only escalate unexpected errors to the logger.
136-152: Season-scope grabbed-history matching will miss individual-episode grabs.When the scope is season-only (no
targetEpisodeId), the filter only matches history entries whereh.data?.seasonNumberequals the target season. However, Sonarr populatesdata.seasonNumberonly on season-pack grabs; individual-episode grabs include only a top-levelepisodeId. The current filter will therefore skip individual-episode grabs, causing the most recent grab for a season composed of single-episode releases to be missed — you'll fall through to the "no grabbed history found" fallback and rely purely on file deletion and re-search.Since you already have
allEpisodes, resolve each history record'sepisodeIdto its season as a secondary match:♻️ Proposed refactor
- const inScope = - scope.seasonNumber !== undefined - ? history.filter( - (h) => - h.eventType === 'grabbed' && - // Sonarr history records include seasonNumber under data for - // season-pack grabs and under top-level for episode grabs. - String(h.data?.seasonNumber ?? '') === String(scope.seasonNumber) - ) - : history.filter((h) => h.eventType === 'grabbed'); - grabbedId = inScope[0]?.id; + const episodeSeasonById = new Map( + allEpisodes.map((e) => [e.id, e.seasonNumber]) + ); + const inScope = + scope.seasonNumber !== undefined + ? history.filter((h) => { + if (h.eventType !== 'grabbed') return false; + // Season-pack grabs: data.seasonNumber is populated. + if ( + String(h.data?.seasonNumber ?? '') === + String(scope.seasonNumber) + ) { + return true; + } + // Single-episode grabs: resolve seasonNumber via episodeId. + return ( + h.episodeId !== undefined && + episodeSeasonById.get(h.episodeId) === scope.seasonNumber + ); + }) + : history.filter((h) => h.eventType === 'grabbed'); + grabbedId = inScope[0]?.id;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/issueRedownload.ts` around lines 136 - 152, The season-scope history filter currently only checks h.data?.seasonNumber and thus misses individual-episode grabs; update the branch where scope.seasonNumber is defined (the code building inScope and setting grabbedId) to also accept history entries with a top-level h.episodeId whose episode's season (resolve via the existing allEpisodes collection/map) equals scope.seasonNumber — i.e., when scope.seasonNumber !== undefined, include entries where String(h.data?.seasonNumber) === String(scope.seasonNumber) OR where h.episodeId maps to an episode in allEpisodes whose seasonNumber matches; leave grabbedId = inScope[0]?.id logic 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 `@server/api/servarr/sonarr.ts`:
- Around line 357-398: searchSeason and searchEpisodes currently catch errors
from this.runCommand('SeasonSearch'|'EpisodeSearch') and only log them, causing
calling code (e.g., triggerRedownload) to assume success; change these methods
to rethrow the caught error after logging (or remove the try/catch and let the
error bubble) so failures propagate to callers. Locate the methods searchSeason
and searchEpisodes and update their catch blocks to log the error (using
e.message) and then throw e (or omit the try/catch entirely) so callers can
detect and handle Sonarr command failures.
In `@server/routes/issue.ts`:
- Around line 325-331: Before calling triggerRedownload(issue) guard the route
by checking the issue status: after retrieving the issue with
issueRepository.findOneOrFail, verify issue.status === IssueStatus.OPEN and if
not respond with an error (e.g., 400/409 and a clear message) or throw an HTTP
error; only call triggerRedownload(issue) when the status is OPEN. Update any
imports/usages to reference IssueStatus enum and ensure the early return happens
before the try/catch that calls triggerRedownload.
---
Nitpick comments:
In `@server/lib/issueRedownload.ts`:
- Around line 80-85: The sequence that marks a grab failed then deletes the file
then triggers a search can leave the system in a half-applied state if
deleteMovieFile/deleteEpisodeFile throws; wrap the delete call(s) (references:
deleteMovieFile, deleteEpisodeFile and the surrounding radarr.getMovie usage) in
a try/catch that logs the error (including 404/not-found cases) and continues to
call radarr.searchMovie so the search is attempted even when the delete fails,
and ensure markGrabAsFailed remains unchanged; treat 404-like errors as
non-fatal and only escalate unexpected errors to the logger.
- Around line 136-152: The season-scope history filter currently only checks
h.data?.seasonNumber and thus misses individual-episode grabs; update the branch
where scope.seasonNumber is defined (the code building inScope and setting
grabbedId) to also accept history entries with a top-level h.episodeId whose
episode's season (resolve via the existing allEpisodes collection/map) equals
scope.seasonNumber — i.e., when scope.seasonNumber !== undefined, include
entries where String(h.data?.seasonNumber) === String(scope.seasonNumber) OR
where h.episodeId maps to an episode in allEpisodes whose seasonNumber matches;
leave grabbedId = inScope[0]?.id logic 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d383ca4d-b2da-4418-917c-52d3a62a9418
📒 Files selected for processing (9)
seerr-api.ymlserver/api/servarr/base.tsserver/api/servarr/radarr.tsserver/api/servarr/sonarr.tsserver/lib/issueRedownload.tsserver/routes/issue.test.tsserver/routes/issue.tssrc/components/IssueDetails/index.tsxsrc/i18n/locale/en.json
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/routes/issue.test.ts (1)
146-149: Assert the redownload service receives the target issue.The mock currently ignores its argument, so this test would still pass if the route called
triggerRedownloadwith the wrong issue. Please assert the mocked call’s first argument has the expected issue id.Suggested assertion
assert.strictEqual(res.status, 200); assert.strictEqual(triggerRedownloadMock.callCount(), 1); + assert.strictEqual(triggerRedownloadMock.calls[0].arguments[0].id, issue.id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/issue.test.ts` around lines 146 - 149, The test only checks that triggerRedownloadMock was called once but doesn't verify the argument; update the test to assert that the mock's first call received the expected issue (check triggerRedownloadMock's first call args and assert the arg's id equals issue.id). Locate the mock named triggerRedownloadMock used after agent.post(`/issue/${issue.id}/redownload`) and add an assertion that the first call's first argument has id equal to issue.id (in addition to the existing call count and response status assertions).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/routes/issue.test.ts`:
- Around line 146-149: The test only checks that triggerRedownloadMock was
called once but doesn't verify the argument; update the test to assert that the
mock's first call received the expected issue (check triggerRedownloadMock's
first call args and assert the arg's id equals issue.id). Locate the mock named
triggerRedownloadMock used after agent.post(`/issue/${issue.id}/redownload`) and
add an assertion that the first call's first argument has id equal to issue.id
(in addition to the existing call count and response status assertions).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e51faca5-5c55-4a40-b30f-c97d313c0cbe
📒 Files selected for processing (3)
server/api/servarr/sonarr.tsserver/routes/issue.test.tsserver/routes/issue.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- server/api/servarr/sonarr.ts
- server/routes/issue.ts
|
Hi folks, really sorry to nudge, and I completely understand you're busy. Just wanted to gently check in on this one in case it slipped off the radar. Totally happy to make any changes, break it up differently, or answer any questions if there's anything that would make review easier. No rush at all, and thank you so much for all the work you put into this project. I really appreciate it. 🙏 |
|
Hey! Waiting for a feature like this |
Thanks for the kind words! For this PR I deliberately scoped it to the Issue Details page so it stays small and focused for review. A bulk retry action from the requests list is a great idea but really a separate feature (different permission boundary, different UX), so probably best filed as a follow up issue once this lands. I'm happy to help look at that next if maintainers are interested. |
2 participants
Description
Adds an admin only action "Mark as failed and redownload" action on the "issue details" page. When an admin clicks it, seer asks radarr/sonarr to mark the currently grabbed release as failed, which blocklists that release and automatically triggers a new search. The issue is auto resolved and a system comment is appended recording the action. Issue creation and the redownload action are on separate permissions so that anyone can still file an issue, but only admins can trigger the redownload.
It now asks radarr/sonarr to delete the scoped file, then kicks off a search at the matching scope. then resolves the issue and appends a system commit logging what was done.
For TV the scope follows the issue's
problemSeasonandproblemEpisodefields. If no grabbed history record can be found, falls back to a plain search.Related open issues this touches:
How Has This Been Tested?
Unit tests with pnpm test. 35/35 pass including the 2 new tests
linting/formatting/i18n all clean
Screenshots / Logs (if applicable)
Checklist:
I have read and followed the contribution guidelines.
Disclosed any use of AI (see our policy)
I have updated the documentation accordingly.
All new and existing tests passed.
Successful build
pnpm buildTranslation keys
pnpm i18n:extractDatabase migration (if required)
AI Disclosure: I used Claude Code to draft the implementation plan and generate the initial code. I reviewed every commit, ran
pnpm test/pnpm lint/pnpm format:checklocally, and performed all manual Radarr/Sonarr verification myself. PR description is my own words.Summary by CodeRabbit
New Features
Behavior / Fixes
Tests
Localization