feat: Add media removal request system with approval workflow, per-season support, and granular permissions#2828
Open
MrDWilson wants to merge 16 commits intoseerr-team:developfrom
Open
feat: Add media removal request system with approval workflow, per-season support, and granular permissions#2828MrDWilson wants to merge 16 commits intoseerr-team:developfrom
MrDWilson wants to merge 16 commits intoseerr-team:developfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a media removal-request feature: OpenAPI endpoints, DB entity and migrations, permission bits and checks, Servarr removal integration (Sonarr season deletions), backend routes for lifecycle actions, and frontend UI/components for creating and managing removal requests (season-level and 4K-aware). Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend
participant API
participant DB
participant Servarr
User->>Frontend: Submit removal request (mediaId, is4k, seasons?)
Frontend->>API: POST /api/v1/removal-request
activate API
API->>DB: Validate, check conflicts, insert request (PENDING or APPROVED)
alt Auto-approved
API->>Servarr: Execute removal (Radarr/Sonarr)
Servarr-->>API: Success / Error
alt Success
API->>DB: Update media state (DELETE or PARTIAL), set request APPROVED
else Error
API->>DB: Set request FAILED
end
end
API-->>Frontend: 201 Created / 409 Conflict / 4xx/5xx
deactivate API
User->>Frontend: Approve / Retry action
Frontend->>API: POST /api/v1/removal-request/{id}/approve (or /retry)
activate API
API->>DB: Transition status -> APPROVED (set modifiedBy)
API->>Servarr: Execute removal
Servarr-->>API: Success / Error
API->>DB: Update request status and media state
API-->>Frontend: 200 OK / 4xx/5xx
deactivate API
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a full media-removal request workflow to Seerr (server + UI + API spec), including per-season TV support, approval/retry/decline lifecycle, and new permission bits (migrated to bigint + BigInt-based checks) to safely support >32-bit permission flags.
Changes:
- Introduces
MediaRemovalRequestdata model, migrations (SQLite/Postgres), and/removal-requestAPI routes for create/list/approve/decline/retry/delete. - Extends UI to create/manage removal requests (manage slide-over, request list, per-season modal, status blocks) and adds i18n strings.
- Adds Sonarr per-season file removal support and expands permissions + permission editor UI.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/i18n/locale/en.json | Adds new UI strings for removal-request workflow and permissions. |
| src/components/TvDetails/index.tsx | Expands “manage” entrypoint visibility to removal-request-capable users (scoped). |
| src/components/RequestModal/SeasonRemovalModal.tsx | New modal for selecting seasons to request removal. |
| src/components/RequestList/index.tsx | Shows a “Pending Removal Requests” section for managers. |
| src/components/RemovalRequestBlock/index.tsx | New UI block for displaying/remediating removal requests (approve/decline/retry/delete). |
| src/components/PermissionOption/index.tsx | Adds childrenAutoCheck support and includes new auto-approve removal permission in logic. |
| src/components/PermissionEdit/index.tsx | Adds new removal permissions to the permission editor with hierarchy/requirements. |
| src/components/MovieDetails/index.tsx | Expands “manage” entrypoint visibility to removal-request-capable users (scoped). |
| src/components/ManageSlideOver/index.tsx | Adds removal request section, per-season flow, and displays removal request history. |
| server/routes/removalRequest.ts | New removal request API endpoints (list/create/approve/decline/retry/delete) with validations and execution. |
| server/routes/index.ts | Registers /removal-request routes. |
| server/migration/sqlite/1775397567178-AddMediaRemovalRequest.ts | SQLite migration adding media_removal_request table and migrating user.permissions to bigint. |
| server/migration/postgres/1775397575694-AddMediaRemovalRequest.ts | Postgres migration adding media_removal_request table and migrating user.permissions to bigint. |
| server/lib/permissions.ts | Adds new permission bits and refactors hasPermission() to use BigInt bitwise ops. |
| server/entity/User.ts | Changes permissions column to bigint with transformer. |
| server/entity/MediaRemovalRequest.ts | New entity encapsulating approval state and execution logic for removals (Radarr/Sonarr + Seerr DB updates). |
| server/entity/Media.ts | Adds relation to removal requests and includes them in getMedia() relations. |
| server/constants/media.ts | Adds MediaRemovalRequestStatus enum. |
| server/api/servarr/sonarr.ts | Adds removeSeasonFiles() to enable per-season TV removal behavior. |
| seerr-api.yml | Documents the new /removal-request endpoints in the OpenAPI spec. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (6)
src/components/TvDetails/index.tsx (1)
697-733: Permission logic is correct but has minor redundancy.The nested permission checks work correctly:
- First condition gates on
MANAGE_REQUESTSORREQUEST_REMOVAL- Second condition further restricts
REQUEST_REMOVALusers to those withREMOVAL_ALLor who own existing requestsHowever, checking
hasPermission(Permission.ADMIN)andhasPermission(Permission.MANAGE_REQUESTS)in the second condition is redundant since:
ADMINusers already pass allhasPermissioncalls (including the first one)MANAGE_REQUESTSusers already satisfied the first condition and would satisfy the second via the explicit checkThis redundancy doesn't cause bugs but could be simplified:
♻️ Optional: Simplify redundant permission checks
- {hasPermission( - [Permission.MANAGE_REQUESTS, Permission.REQUEST_REMOVAL], - { type: 'or' } - ) && - (hasPermission(Permission.REMOVAL_ALL) || - hasPermission(Permission.MANAGE_REQUESTS) || - hasPermission(Permission.ADMIN) || - data.mediaInfo?.requests?.some( - (r) => r.requestedBy.id === user?.id - )) && - data.mediaInfo && ( + {(hasPermission(Permission.MANAGE_REQUESTS) || + (hasPermission(Permission.REQUEST_REMOVAL) && + (hasPermission(Permission.REMOVAL_ALL) || + data.mediaInfo?.requests?.some( + (r) => r.requestedBy.id === user?.id + )))) && + data.mediaInfo && (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/TvDetails/index.tsx` around lines 697 - 733, Remove the redundant permission checks in the nested condition inside the TvDetails component: keep the outer guard hasPermission([Permission.MANAGE_REQUESTS, Permission.REQUEST_REMOVAL], { type: 'or' }) and simplify the inner condition to only check hasPermission(Permission.REMOVAL_ALL) || data.mediaInfo?.requests?.some(r => r.requestedBy.id === user?.id); remove the extra hasPermission(Permission.ADMIN) and hasPermission(Permission.MANAGE_REQUESTS) checks so the rendered Tooltip/Button logic still requires either removal-all permission or ownership of an existing request.seerr-api.yml (1)
6275-6287: Document these responses with a concreteMediaRemovalRequestschema.Right now the list items and create/approve/decline/retry responses are all opaque
objects, so the spec never tells clients about fields likestatus,requestedBy,is4k,seasons, or timestamps. Add a reusable schema here and reference it from these operations so the new API surface is actually typed and much harder to drift from the server.Also applies to: 6318-6325, 6340-6345, 6360-6365, 6380-6385
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@seerr-api.yml` around lines 6275 - 6287, Replace the opaque inline object responses for the removal endpoints with a reusable MediaRemovalRequest schema: change the '200' response's content -> application/json -> schema -> properties -> results -> items to reference '#/components/schemas/MediaRemovalRequest' (instead of type: object) and update the create/approve/decline/retry responses similarly; add a concrete components.schemas.MediaRemovalRequest definition that enumerates fields like id, status, requestedBy, is4k, seasons, createdAt, updatedAt, etc., and reference that schema for all affected response bodies (also update the same pattern at the other response blocks noted in the review: the ranges around 6318-6325, 6340-6345, 6360-6365, 6380-6385) so clients receive typed responses.server/entity/MediaRemovalRequest.ts (1)
116-121: Redundant TMDB API call whentvdbIdalready exists.Consider checking
media.tvdbIdfirst to avoid the TMDB API call when the ID is already available:♻️ Proposed optimization
} else { - const tmdb = new TheMovieDb(); - const series = await tmdb.getTvShow({ tvId: media.tmdbId }); - const tvdbId = series.external_ids.tvdb_id ?? media.tvdbId; + let tvdbId = media.tvdbId; + if (!tvdbId) { + const tmdb = new TheMovieDb(); + const series = await tmdb.getTvShow({ tvId: media.tmdbId }); + tvdbId = series.external_ids.tvdb_id; + } if (!tvdbId) { throw new Error('TVDB ID not found'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/entity/MediaRemovalRequest.ts` around lines 116 - 121, Avoid the redundant TMDB API call by using media.tvdbId first: check if media.tvdbId exists before instantiating TheMovieDb or calling getTvShow; only create const tmdb and call tmdb.getTvShow({ tvId: media.tmdbId }) to derive tvdbId when media.tvdbId is missing, then set tvdbId accordingly and preserve the existing error throw when no tvdbId is found. Reference symbols: media.tvdbId, TheMovieDb, tmdb, getTvShow, series, tvdbId.server/migration/sqlite/1775397567178-AddMediaRemovalRequest.ts (2)
6-91: Redundant table rebuilds in up() migration.The migration rebuilds
user_push_subscriptiontwice (lines 8-20 and 44-57) andusertable twice (lines 21-28 and 58-65). The first rebuild of each table doesn't change the schema - the actualpermissionscolumn change fromintegertobigintonly happens in the second rebuild (line 59).This doubles the migration time and data copying unnecessarily. Consider consolidating to a single rebuild per table.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/migration/sqlite/1775397567178-AddMediaRemovalRequest.ts` around lines 6 - 91, The up() migration performs duplicate rebuilds of user_push_subscription and user (creating temporary_user_push_subscription/temporary_user twice and temporary_user/temporary_user twice) causing unnecessary data copies; remove the first set of DROP/CREATE/INSERT/DROP/ALTER statements for user_push_subscription and user and keep the single rebuild that introduces the permissions change to bigint (the block that creates temporary_user with permissions bigint), ensuring you still recreate the IDX_03f7958328e311761b0de675fb index and preserve the foreign key constraints on temporary_user_push_subscription and any index recreation for user_push_subscription/user.
143-147: Duplicate DROP INDEX statements in down() migration.Lines 95-98 already drop these same indexes (
IDX_34c6963994828cb30c9b2798df,IDX_148182cef7f27b27b1fdacd7de,IDX_78decd4e1901d80cfdce43b079,IDX_64e0da8892d7f8aabce7198097), then lines 109-120 recreate them, and lines 143-146 try to drop them again before dropping the table at line 147.This appears to work but is unnecessarily complex. The intermediate recreation (lines 99-120) that strips FKs from
media_removal_requestbefore finally dropping it could be simplified to just dropping the table directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/migration/sqlite/1775397567178-AddMediaRemovalRequest.ts` around lines 143 - 147, The down() method in the AddMediaRemovalRequest migration contains duplicated DROP INDEX calls and an unnecessary recreate-block; simplify it by removing the intermediate recreation and the duplicate DROP INDEX statements so that down() either drops the indexes once (using the existing index names IDX_34c6963994828cb30c9b2798df, IDX_148182cef7f27b27b1fdacd7de, IDX_78decd4e1901d80cfdce43b079, IDX_64e0da8892d7f8aabce7198097) then calls queryRunner.query(`DROP TABLE "media_removal_request"`) or, if the DB will cascade, simply call DROP TABLE "media_removal_request" directly; update the queryRunner.query calls inside the AddMediaRemovalRequest.down() method to remove the duplicated drops and the recreate block so the migration is concise and only drops each index/table once.server/routes/removalRequest.ts (1)
120-132: Consider validating season 0 (specials) consistently with UI.The API accepts season 0 (
s >= 0), butSeasonRemovalModalexplicitly filters out season 0 (specials). If the intention is to disallow removal of specials, consider addings > 0validation here. Otherwise, document that season 0 removal is supported via direct API calls but not through the UI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/removalRequest.ts` around lines 120 - 132, The current seasons validation in removalRequest.ts allows season 0 (checks s < 0); to match the UI's SeasonRemovalModal which excludes specials, change the validation predicate so seasons.some((s) => typeof s !== 'number' || !Number.isInteger(s) || s <= 0) and update the error message to "Seasons must be an array of positive integers." Ensure this check lives in the same request handler that references the seasons variable so API rejects season 0 consistently.
🤖 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 440-460: The code deletes episode files and updates
series.seasons.monitored but fails to unmonitor individual episodes first, which
can allow Sonarr to re-import missing files; modify the flow in this block so
that after computing episodeFileIds (from getEpisodes) you explicitly set the
affected episodes' monitored flag to false (use the same episode list filtered
by episodeFileId/seasonNumbers) by calling the existing monitorEpisodes or
equivalent API to update those episodes' monitored state before calling
axios.delete for each episodeFileId, then proceed to update series via
axios.put; reference functions/fields: getEpisodes, episodeFileId,
monitorEpisodes, axios.delete, axios.put, and series.seasons in your changes.
In `@server/entity/MediaRemovalRequest.ts`:
- Around line 127-131: The call to service.removeSeasonFiles(tvdbId,
this.seasons!) can partially succeed (files deleted but unmonitor PUT failed)
which leaves Sonarr state inconsistent and the DB transaction aborted; wrap the
call in a try/catch in MediaRemovalRequest (the branch that currently calls
removeSeasonFiles/removeSeries) and handle partial failures by logging a
structured, descriptive message including tvdbId and seasons and the partial
outcome; preferably change removeSeasonFiles to return a status object (or throw
a PartialFailureError) indicating steps completed (e.g., { filesDeleted: true,
unmonitorSucceeded: false }) and, in MediaRemovalRequest, detect that status and
emit a clear warning/error log so retries/ops can observe the partial state
before the DB transaction is aborted.
- Around line 72-77: The updatedAt field in MediaRemovalRequest is using
`@DbAwareColumn` with an unsupported onUpdate option so it never auto-updates;
change the decorator to TypeORM's `@UpdateDateColumn`() on the updatedAt property,
add UpdateDateColumn to the import list (alongside
Column/Entity/PrimaryGeneratedColumn/etc.), and remove the onUpdate/default
options from the current `@DbAwareColumn` usage so updatedAt is managed
automatically by TypeORM; keep the property type as Date and ensure no manual
setting is required elsewhere in MediaRemovalRequest.
In `@server/migration/postgres/1775397575694-AddMediaRemovalRequest.ts`:
- Around line 47-48: The down migration is unsafe because PostgreSQL integer is
32-bit and the new permission constants AUTO_APPROVE_REMOVAL (2147483648) and
REMOVAL_ALL (4294967296) can exceed its range; update the migration file
AddMediaRemovalRequest.ts so the down() path clears/masks these high bits on the
"permissions" column before casting back to integer (e.g. run an UPDATE to
remove AUTO_APPROVE_REMOVAL and REMOVAL_ALL bits from "permissions"), then
perform the ALTER TABLE "user" ALTER COLUMN "permissions" TYPE integer;
alternatively, if you prefer irreversible down(), explicitly throw/mark the down
migration as non-reversible in the same file.
In `@src/components/ManageSlideOver/index.tsx`:
- Around line 131-139: availableSeasons is currently computed and passed to
SeasonRemovalModal only for non-4K (is4k=false), so the UI never surfaces
4K-only season removal; update the logic that builds availableSeasons (and the
code paths around lines referenced) to compute two sets or a parameterized set
depending on an is4k flag on the modal, and pass is4k={true} when the user
toggles or selects the 4K branch so SeasonRemovalModal can show 4K seasons;
specifically modify the branch that inspects mediaType and
data.mediaInfo?.seasons to include seasons whose status/is4k match the requested
removal mode, and ensure the SeasonRemovalModal component invocation
(SeasonRemovalModal) receives the computed availableSeasons and the correct is4k
prop for the 4K UI path.
In `@src/components/RemovalRequestBlock/index.tsx`:
- Around line 171-180: The delete button is shown for all rows even though
server-side (server/routes/removalRequest.ts) only allows the requester or users
with the MANAGE_REQUESTS role plus Permission.REQUEST_REMOVAL to delete; update
RemovalRequestBlock to only render the Tooltip/Button when the current user is
allowed: check that either currentUser.id === request.requesterId (or
request.requester?.id) OR the current user has the MANAGE_REQUESTS
role/Permission.REQUEST_REMOVAL (use your existing permission/role helper or
prop that indicates permissions), and only call deleteRequest() when that
conditional passes. This prevents rendering a button that will always 403 and
keeps deleteRequest as the click handler for authorized cases.
- Around line 136-154: The icon-only buttons (approve/decline and the delete
button) lack accessible names for screen readers; update the Button usages that
render CheckIcon, XMarkIcon (and the delete button at the other block) to
include accessible labels by either adding an aria-label prop with the localized
string (e.g. aria-label={intl.formatMessage(messages.approve)} /
messages.decline / messages.delete) or inserting a visually-hidden text node
(e.g. a span with the sr-only class containing intl.formatMessage(...)) inside
the Button; target the Button components wrapping CheckIcon, XMarkIcon and the
delete icon and use updateRequest and the existing messages keys for the label
text.
In `@src/components/RequestModal/SeasonRemovalModal.tsx`:
- Around line 122-125: The onKeyDown handler is checking for the wrong Space key
string ("Space") so pressing the spacebar won't trigger toggleAllSeasons; update
the condition in the onKeyDown callback (where toggleAllSeasons() is called) to
detect space correctly by checking e.key === ' ' (a single space character) or
alternatively check e.code === 'Space' for robustness, then call
toggleAllSeasons() when either condition is true.
- Around line 182-186: The per-row keyboard handler uses e.key === 'Space' which
is unreliable; update the onKeyDown handler to detect the Space key robustly
(e.g., check e.code === 'Space' in addition to Enter) and call
e.preventDefault() when handling Space/Enter before invoking
toggleSeason(season.seasonNumber) so the action triggers consistently without
causing page scrolling; modify the onKeyDown arrow function that currently
references toggleSeason and season.seasonNumber accordingly.
---
Nitpick comments:
In `@seerr-api.yml`:
- Around line 6275-6287: Replace the opaque inline object responses for the
removal endpoints with a reusable MediaRemovalRequest schema: change the '200'
response's content -> application/json -> schema -> properties -> results ->
items to reference '#/components/schemas/MediaRemovalRequest' (instead of type:
object) and update the create/approve/decline/retry responses similarly; add a
concrete components.schemas.MediaRemovalRequest definition that enumerates
fields like id, status, requestedBy, is4k, seasons, createdAt, updatedAt, etc.,
and reference that schema for all affected response bodies (also update the same
pattern at the other response blocks noted in the review: the ranges around
6318-6325, 6340-6345, 6360-6365, 6380-6385) so clients receive typed responses.
In `@server/entity/MediaRemovalRequest.ts`:
- Around line 116-121: Avoid the redundant TMDB API call by using media.tvdbId
first: check if media.tvdbId exists before instantiating TheMovieDb or calling
getTvShow; only create const tmdb and call tmdb.getTvShow({ tvId: media.tmdbId
}) to derive tvdbId when media.tvdbId is missing, then set tvdbId accordingly
and preserve the existing error throw when no tvdbId is found. Reference
symbols: media.tvdbId, TheMovieDb, tmdb, getTvShow, series, tvdbId.
In `@server/migration/sqlite/1775397567178-AddMediaRemovalRequest.ts`:
- Around line 6-91: The up() migration performs duplicate rebuilds of
user_push_subscription and user (creating
temporary_user_push_subscription/temporary_user twice and
temporary_user/temporary_user twice) causing unnecessary data copies; remove the
first set of DROP/CREATE/INSERT/DROP/ALTER statements for user_push_subscription
and user and keep the single rebuild that introduces the permissions change to
bigint (the block that creates temporary_user with permissions bigint), ensuring
you still recreate the IDX_03f7958328e311761b0de675fb index and preserve the
foreign key constraints on temporary_user_push_subscription and any index
recreation for user_push_subscription/user.
- Around line 143-147: The down() method in the AddMediaRemovalRequest migration
contains duplicated DROP INDEX calls and an unnecessary recreate-block; simplify
it by removing the intermediate recreation and the duplicate DROP INDEX
statements so that down() either drops the indexes once (using the existing
index names IDX_34c6963994828cb30c9b2798df, IDX_148182cef7f27b27b1fdacd7de,
IDX_78decd4e1901d80cfdce43b079, IDX_64e0da8892d7f8aabce7198097) then calls
queryRunner.query(`DROP TABLE "media_removal_request"`) or, if the DB will
cascade, simply call DROP TABLE "media_removal_request" directly; update the
queryRunner.query calls inside the AddMediaRemovalRequest.down() method to
remove the duplicated drops and the recreate block so the migration is concise
and only drops each index/table once.
In `@server/routes/removalRequest.ts`:
- Around line 120-132: The current seasons validation in removalRequest.ts
allows season 0 (checks s < 0); to match the UI's SeasonRemovalModal which
excludes specials, change the validation predicate so seasons.some((s) => typeof
s !== 'number' || !Number.isInteger(s) || s <= 0) and update the error message
to "Seasons must be an array of positive integers." Ensure this check lives in
the same request handler that references the seasons variable so API rejects
season 0 consistently.
In `@src/components/TvDetails/index.tsx`:
- Around line 697-733: Remove the redundant permission checks in the nested
condition inside the TvDetails component: keep the outer guard
hasPermission([Permission.MANAGE_REQUESTS, Permission.REQUEST_REMOVAL], { type:
'or' }) and simplify the inner condition to only check
hasPermission(Permission.REMOVAL_ALL) || data.mediaInfo?.requests?.some(r =>
r.requestedBy.id === user?.id); remove the extra hasPermission(Permission.ADMIN)
and hasPermission(Permission.MANAGE_REQUESTS) checks so the rendered
Tooltip/Button logic still requires either removal-all permission or ownership
of an existing request.
🪄 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: a856ee6b-0247-42b2-ac9a-d17b0024d497
📒 Files selected for processing (20)
seerr-api.ymlserver/api/servarr/sonarr.tsserver/constants/media.tsserver/entity/Media.tsserver/entity/MediaRemovalRequest.tsserver/entity/User.tsserver/lib/permissions.tsserver/migration/postgres/1775397575694-AddMediaRemovalRequest.tsserver/migration/sqlite/1775397567178-AddMediaRemovalRequest.tsserver/routes/index.tsserver/routes/removalRequest.tssrc/components/ManageSlideOver/index.tsxsrc/components/MovieDetails/index.tsxsrc/components/PermissionEdit/index.tsxsrc/components/PermissionOption/index.tsxsrc/components/RemovalRequestBlock/index.tsxsrc/components/RequestList/index.tsxsrc/components/RequestModal/SeasonRemovalModal.tsxsrc/components/TvDetails/index.tsxsrc/i18n/locale/en.json
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/migration/postgres/1775397575694-AddMediaRemovalRequest.ts`:
- Around line 25-27: The FK on media_removal_request currently uses ON DELETE
CASCADE which causes removal requests to vanish when Media is deleted; update
the ALTER TABLE statement that creates constraint
"FK_78decd4e1901d80cfdce43b079f" (the FK on "mediaId" referencing "media"("id"))
so it does not cascade on delete (use ON DELETE NO ACTION or RESTRICT, or ON
DELETE SET NULL if "mediaId" is nullable) to preserve the MediaRemovalRequest
rows after Media deletion; change the SQL in AddMediaRemovalRequest migration
accordingly and keep the FK name and referenced columns the same.
In `@server/routes/removalRequest.ts`:
- Around line 127-139: The seasons validation currently allows an empty array
and doesn't verify selected seasons exist on the target media; update the
validation in removalRequest.ts so that when seasons is provided it must be a
non-empty array of positive integers and every season id must match an existing
season on the loaded media. Specifically: before you build the removal request,
load the media with its seasons (use the same media-loading codepath already
used later), then replace the loose check around the seasons variable with:
reject if !Array.isArray(seasons) or seasons.length === 0 or any element is not
a positive integer, and reject if any season id is not found in media.seasons
(call next({status:400, message:...}) on failure). Apply the same stricter check
to the other seasons validation sites referenced (the blocks around the earlier
141-155 range and the later logic that currently rewrites [] to undefined) and
ensure you do not coerce an empty array into undefined.
In `@src/components/ManageSlideOver/index.tsx`:
- Around line 350-366: The requestRemoval function swallows every 409 and treats
them as "already pending"; instead, only handle the specific duplicate-pending
409 and let other 409 conflicts bubble up. Update requestRemoval (the axios.post
call and its catch) to inspect e.response?.data (e.g., a specific error
code/property such as data.code | data.type | data.reason) and only suppress the
error and proceed to revalidate()/onClose() when that value matches the server's
duplicate-pending identifier; for any other 409 or unknown response shape
rethrow the error. Also move revalidate() and onClose() into the
successful/suppressed branch so they only run when the request truly succeeded
or is the known duplicate-pending case.
🪄 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: 18225dd5-bcf1-4b7b-84ca-d82fed89a7e9
📒 Files selected for processing (8)
server/api/servarr/sonarr.tsserver/entity/MediaRemovalRequest.tsserver/entity/User.tsserver/migration/postgres/1775397575694-AddMediaRemovalRequest.tsserver/routes/removalRequest.tssrc/components/ManageSlideOver/index.tsxsrc/components/RemovalRequestBlock/index.tsxsrc/components/RequestModal/SeasonRemovalModal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- server/entity/User.ts
1 task
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
server/routes/removalRequest.ts (1)
131-147:⚠️ Potential issue | 🔴 CriticalValidate season requests against loaded seasons before they can execute.
seasons: []still passes validation and is rewritten toundefinedat Line 230, which can silently promote a season removal into a full-media removal. Also, these season-removal paths never loadmedia.seasons, butMediaRemovalRequest.executeRemoval()uses that relation to update Seerr season status, so an approved season request can remove files externally without marking the matching local seasons deleted.Also applies to: 226-230, 279-281, 365-367
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/removalRequest.ts` around lines 131 - 147, The seasons validation currently allows an empty array which later gets coerced to undefined and can turn a season-only removal into a full-media removal; update the validation around mediaRepository.findOne so that seasons must be a non-empty array of positive integers (reject []), do not rewrite seasons to undefined, and when handling season-specific removals load the Media.seasons relation (e.g., via mediaRepository.findOne with relations) so MediaRemovalRequest.executeRemoval() has the loaded media.seasons to update local season status; apply the same fix to the other validation sites referenced (around the logic at lines handling seasons → 226-230, 279-281, 365-367).
🧹 Nitpick comments (2)
seerr-api.yml (1)
6287-6294: Define a concrete removal-request schema instead of anonymous objects.All of the new responses here are just
type: object, so generated clients lose fields likestatus,is4k,seasons,reason,media,requestedBy, andmodifiedBy. Please add a reusableMediaRemovalRequestschema plus a paginated list wrapper and reference that from these endpoints.Also applies to: 6331-6332, 6355-6356, 6375-6376, 6395-6396
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@seerr-api.yml` around lines 6287 - 6294, Create a concrete MediaRemovalRequest schema (include fields status, is4k, seasons, reason, media, requestedBy, modifiedBy and their appropriate types) and then define a paginated wrapper schema (e.g., MediaRemovalRequestList) that has pageInfo: $ref '#/components/schemas/PageInfo' and results: type: array, items: $ref '#/components/schemas/MediaRemovalRequest'; replace the anonymous object usages under results in the affected response definitions (the blocks currently using type: object) with $ref to MediaRemovalRequestList so all endpoints reference the reusable schemas instead of anonymous objects.server/routes/removalRequest.ts (1)
20-27: Let the OpenAPI layer own query validation here.
take/skipare being clamped locally and unknownfiltervalues fall back to"all", which silently rewrites invalid requests instead of rejecting them. This router family intentionally relies on the upstream OpenAPI validator for query enforcement, so it would be better to keep only defaults for omitted values and read the validated params directly.Based on learnings: In server/routes/**/*.ts, rely on upstream OpenAPI validator middleware for query parameters (e.g., page). Route handlers should not defensively validate or clamp query parameter values, as this is intentional and aligns with the project convention.
Also applies to: 31-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/removalRequest.ts` around lines 20 - 27, This code clamps and silently rewrites query params (rawTake/rawSkip -> pageSize/skip and defaulting invalid filter to "all"); instead let the OpenAPI validator enforce types/limits and read the validated values directly from req.query, only providing defaults when a param is omitted. Remove the Number.isFinite checks, Math.floor/clamping logic and any fallback-for-invalid behavior in the handler (variables: rawTake, rawSkip, pageSize, skip, and filter), and simply use the values supplied by the OpenAPI-validated req.query (or apply an explicit default only if the query key is undefined).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@seerr-api.yml`:
- Around line 6251-6252: Update the OpenAPI description for the GET
removal-requests endpoint to match the handler logic in
server/routes/removalRequest.ts by including the REQUEST_VIEW permission as
allowed to list/filter removal requests globally (in addition to ADMIN and
MANAGE_REQUESTS); locate the GET removal requests operation description entries
(the two places corresponding to the earlier and later description blocks) and
change the text to state that users with ADMIN, MANAGE_REQUESTS, or REQUEST_VIEW
can return all removal requests, while other users only see their own.
In `@server/routes/removalRequest.ts`:
- Around line 313-319: The catch-all catch blocks in the approve/retry/delete
handlers (where logger.error is called and next({ status: 404, message: 'Removal
request not found.' }) is invoked) incorrectly convert any failure into a 404;
update each catch to detect a true not-found (e.g., check for a specific
NotFoundError type, error.name === 'NotFoundError', or error.status === 404) and
only call next({ status: 404, message: 'Removal request not found.' }) in that
case; for all other errors log the full error (using logger.error with the error
object) and propagate a 500 by calling next(err) or next({ status: 500, message:
'Internal server error.' }) so save/remove/runtime failures aren’t masked.
- Around line 183-189: The de-duplication is not atomic: replace the separate
find + insert/approve + executeRemoval flow with an atomic DB operation (use a
transaction or a unique partial index plus upsert) so two submissions cannot
pass the check simultaneously; specifically, modify the code paths that use
removalRequestRepository.find and then create/approve (the blocks around
removalRequestRepository, executeRemoval, and
MediaRemovalRequestStatus.PENDING/APPROVED) to either (a) run inside
removalRequestRepository.manager.transaction and acquire a row/table lock
(SELECT ... FOR UPDATE) to check for existing PENDING/APPROVED requests and then
insert/update and call executeRemoval, or (b) add a unique partial index on
(media_id, is4k) for relevant statuses and perform an insert with ON CONFLICT
handling (upsert) to ensure only one request is created/approved before
executeRemoval; apply the same change to the other occurrences you noted (the
blocks at the other call sites).
In `@src/components/RequestList/index.tsx`:
- Around line 336-360: The empty-state "No results" is shown even when
removalData.results has items; update the conditional that renders the
empty-state in the RequestList component to check both data.results and
removalData.results (e.g., only show "No results" when data?.results.length ===
0 && removalData?.results.length === 0). Locate the two empty-state render
blocks around the existing removal list (uses removalData and
removalData.results) and the other block referenced (uses data and data.results,
around lines ~373-391) and change their gating logic to require both lists to be
empty before rendering the empty-state.
---
Duplicate comments:
In `@server/routes/removalRequest.ts`:
- Around line 131-147: The seasons validation currently allows an empty array
which later gets coerced to undefined and can turn a season-only removal into a
full-media removal; update the validation around mediaRepository.findOne so that
seasons must be a non-empty array of positive integers (reject []), do not
rewrite seasons to undefined, and when handling season-specific removals load
the Media.seasons relation (e.g., via mediaRepository.findOne with relations) so
MediaRemovalRequest.executeRemoval() has the loaded media.seasons to update
local season status; apply the same fix to the other validation sites referenced
(around the logic at lines handling seasons → 226-230, 279-281, 365-367).
---
Nitpick comments:
In `@seerr-api.yml`:
- Around line 6287-6294: Create a concrete MediaRemovalRequest schema (include
fields status, is4k, seasons, reason, media, requestedBy, modifiedBy and their
appropriate types) and then define a paginated wrapper schema (e.g.,
MediaRemovalRequestList) that has pageInfo: $ref '#/components/schemas/PageInfo'
and results: type: array, items: $ref
'#/components/schemas/MediaRemovalRequest'; replace the anonymous object usages
under results in the affected response definitions (the blocks currently using
type: object) with $ref to MediaRemovalRequestList so all endpoints reference
the reusable schemas instead of anonymous objects.
In `@server/routes/removalRequest.ts`:
- Around line 20-27: This code clamps and silently rewrites query params
(rawTake/rawSkip -> pageSize/skip and defaulting invalid filter to "all");
instead let the OpenAPI validator enforce types/limits and read the validated
values directly from req.query, only providing defaults when a param is omitted.
Remove the Number.isFinite checks, Math.floor/clamping logic and any
fallback-for-invalid behavior in the handler (variables: rawTake, rawSkip,
pageSize, skip, and filter), and simply use the values supplied by the
OpenAPI-validated req.query (or apply an explicit default only if the query key
is undefined).
🪄 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: 0f4f17a0-5d1b-469b-98c8-183b862dacb1
📒 Files selected for processing (5)
seerr-api.ymlserver/entity/Media.tsserver/entity/User.tsserver/routes/removalRequest.tssrc/components/RequestList/index.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- server/entity/Media.ts
- server/entity/User.ts
|
For auto-approving removal-requests, what happens if multiple users requested something? For example, if User1 and User2 both request MovieA, then User1 requests it be deleted. Does it delete it from Radarr? Remove the User1 tag from MovieA in Radarr then delete it only if no tags remain? Or does it not consider multiple users requesting the same thing at all and just let any user delete any other's requested media (even if doing so might require an initial request of that media before a deletion request is allowed)? |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/entity/MediaRemovalRequest.ts`:
- Around line 150-161: The loop in MediaRemovalRequest.executeRemoval uses
media.seasons which may be undefined because seasons is not an eager relation;
update executeRemoval (or the code path that handles the season-removal branch)
to explicitly load the seasons relation before iterating — e.g., re-fetch the
Media entity with relations:['seasons'] or use the media repository to load
seasons for media.id, then proceed to set season.status or season.status4k and
call seasonRepository.save(season) as currently done; reference executeRemoval,
MediaRemovalRequest, media.seasons, seasonRepository.save, and is4k when making
the change.
In `@server/routes/removalRequest.ts`:
- Around line 297-299: The removalRequest retrieval uses
removalRequestRepository.findOneOrFail without loading nested relations, so
media.seasons is missing and executeRemoval() can fail for season-level
removals; update the findOneOrFail call to include the nested relation (e.g.,
pass relations including "media" and "media.seasons") so the returned
removalRequest has media.seasons populated before calling executeRemoval().
- Around line 176-196: The ownership check currently queries
mediaRequestRepository.findOne for media id only, so a user who requested the
non‑4K variant can remove the 4K (or vice versa); update the findOne where
clause to also match the 4K variant by including the is4k condition (e.g. add
is4k: media.is4k at the same level as requestedBy, or if is4k is a property of
the related media entity put is4k inside media: { id: media.id, is4k: media.is4k
}) so the query only returns a MediaRequest when the original request matches
the same is4k value; keep the existing permission checks
(Permission.REMOVAL_ALL, Permission.ADMIN, hasPermission) around this lookup.
- Around line 389-391: The code fetches a removalRequest via
removalRequestRepository.findOneOrFail without loading the media.seasons
relation, causing season-level retries to fail; update the call to include the
needed relation (e.g., pass relations: ['media', 'media.seasons'] or the
equivalent nested relation syntax) so that the returned removalRequest has
media.seasons populated before any season-level retry logic executes.
🪄 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: c69793bc-ad17-43ad-9162-46a6a5bd64d6
📒 Files selected for processing (6)
seerr-api.ymlserver/entity/MediaRemovalRequest.tsserver/migration/postgres/1775397575694-AddMediaRemovalRequest.tsserver/migration/sqlite/1775397567178-AddMediaRemovalRequest.tsserver/routes/removalRequest.tssrc/components/RequestList/index.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/RequestList/index.tsx
- server/migration/postgres/1775397575694-AddMediaRemovalRequest.ts
- seerr-api.yml
- server/migration/sqlite/1775397567178-AddMediaRemovalRequest.ts
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 `@server/routes/removalRequest.ts`:
- Around line 17-19: The GET handler registered with
removalRequestRoutes.get('/') is missing the isAuthenticated() middleware;
update the route to include isAuthenticated() before the async (req, res, next)
handler so unauthenticated requests are blocked and req.user is guaranteed,
preventing the subsequent req.user?.hasPermission(...) and the userId filter
from receiving undefined; locate the route registration for
removalRequestRoutes.get and insert the isAuthenticated middleware reference
there.
🪄 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: 02541208-feb8-42f2-948d-56f96ed1f2ca
📒 Files selected for processing (1)
server/routes/removalRequest.ts
Author
This is a very good point that I hadn't considered, and I've now addressed it in my most recent commit. The behaviour, for tagging, is now that all users who originally requested must request removal for the media to actually be removed. The system will track who has approved removal requests, and show the media in an intermediate state. |
Thanks for your work on this feature :) Two more questions along these lines though; what if an admin added a users request manually in Radarr but tagged it appropriately? For example, maybe User1 wanted Movie1 and an admin added it in Radarr then used the Seerr-created Radarr rag for User1 to tag it, but didn't bother creating an associated Seerr request? Does your system check Radarr/Sonare tags? Is there an option for that? Question 2: What happens with requests for different seasons of series? Like User1 requests season 1 of Series1, User2 requests season 2 and 3 of Series1, then User2 requests deletion of season 2 and User1 requests deletion of season 1? Or is it only at the series level, not the season level? |
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
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.
Description
Introduces a complete media removal request workflow, allowing users to request the deletion of movies or individual TV seasons from Seerr and the connected Sonarr/Radarr instances. Requests go through an approval pipeline (pending → approved/declined) controlled by a new set of permissions, with optional auto-approval for privileged users.
Important
Due to adding new permissions, the limit of bitwise ops was hit. To solve this, a migration has been put in place and code updated to use bigint. More details below, but this is an area worth paying special attention to.
New Functionality
Removal Request Entity & Data Model
MediaRemovalRequestentity with status tracking (PENDING,APPROVED,DECLINED,FAILED), 4K awareness, optional per-season targeting, and a free-text reason field.media_removal_requesttable with appropriate indexes and foreign keys.API Endpoints (
/removal-request)GET/POST/REQUEST_REMOVALPOST/:id/approveMANAGE_REQUESTSPOST/:id/declineMANAGE_REQUESTSPOST/:id/retryMANAGE_REQUESTSDELETE/:idMANAGE_REQUESTSSonarr Integration
removeSeasonFiles()method onSonarrAPIthat deletes episode files for specified seasons and unmonitors those seasons, enabling granular per-season removal without deleting the entire series.Permissions
REQUEST_REMOVAL(can create requests),AUTO_APPROVE_REMOVAL(requests are immediately executed),REMOVAL_ALL(can request removal of any media, not just self-requested).hasPermission()refactored to useBigIntfor bitwise operations, necessary because the new permission values exceed 2^30 (the safe limit for 32-bit integer bitwise ops in JS).User.permissionscolumn type changed fromintegertobigintto accommodate the expanded permission space.Frontend
ManageSlideOverextended with removal request creation and management (approve/decline/retry/delete) for both movies and TV shows.RemovalRequestBlockcomponent for displaying removal request status on media detail pages.SeasonRemovalModalcomponent for selecting specific seasons to request removal of.OpenAPI Spec
Risks & Considerations
User.permissionscolumn changes fromintegertobigint. This is a schema migration that affects every user row. The migration should be tested against production-sized databases to confirm no issues with existing permission values.Files Changed (19 files, +1,852 / -39)
Server: New entity, migration (SQLite + Postgres), API routes, Sonarr season removal, permission expansion with BigInt refactor
Frontend: Permission editor, manage slide-over, removal request block, season removal modal, request list integration
Spec: OpenAPI documentation for all new endpoints
How Has This Been Tested?
All existing tests ran and passed, changes thoroughly dev tested with a local running instance, including run through of all possible scenarios and permission combinations.
Screenshots / Logs (if applicable)
Checklist:
pnpm buildpnpm i18n:extractSummary by CodeRabbit
New Features
Permissions
UI