fix: preserve blocklist on media deletion & optimise watchlist-sync#2478
fix: preserve blocklist on media deletion & optimise watchlist-sync#2478fallenbagel wants to merge 3 commits intodevelopfrom
Conversation
…watchlist sync Blocklisted media was being re-requested via watchlist sync because deleting media from Sonarr/Radarr through the UI also deleted the Media entity and its associated Blocklist record via cascade. The media delete endpoint now preserves blocklisted Media entities by clearing service fields instead of removing the row. Additionally, the watchlist sync filter now excludes blocklisted items and skips media that already has an existing auto-request for the user, avoiding unnecessary API calls and database queries on every sync cycle. fix #2475
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds filtering in watchlist sync to skip items the user has already auto-requested by querying existing MediaRequest records, and changes DELETE /:id to preserve BLOCKLISTED media by nulling service-related fields instead of deleting the record. Changes
Sequence Diagram(s)sequenceDiagram
participant Plex as Plex Watchlist API
participant Job as Watchlist Sync Job
participant DB as Database
participant Creator as MediaRequest Creator
Plex->>Job: return watchlist items (tmdbId, mediaType)
Job->>DB: query MediaRequest where userId = X and tmdbId IN (watchlistTmdbIds)
DB-->>Job: existing auto-request set (mediaType:tmdbId)
Job->>Job: filter watchlist items excluding existing set and blocked/unavailable
Job->>Creator: create media request for each remaining item
Creator-->>DB: persist new MediaRequest (if created)
Creator-->>Job: return result (success/failure)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/lib/watchlistsync.ts`:
- Around line 73-84: existingAutoRequests mapping assumes every MediaRequest has
a loaded media relation and accessing r.media.tmdbId can throw; update the
mapping that builds autoRequestedTmdbIds to guard against missing media by
filtering or null-checking each request first (e.g., use
existingAutoRequests.filter(r => r.media).map(r => r.media.tmdbId) or
conditional access) so only valid tmdbId values are added; modify the logic
around requestRepository, existingAutoRequests, and autoRequestedTmdbIds to
safely handle requests with null/undefined media.
There was a problem hiding this comment.
Pull request overview
Fixes a watchlist-sync + media deletion edge case where deleting a Media row could cascade-delete its Blocklist entry, allowing future watchlist sync cycles to recreate and auto-approve requests that should remain blocked.
Changes:
- Updates the media delete endpoint to preserve blocklisted media by nulling service-related fields instead of deleting the row.
- Hardens watchlist sync filtering to skip blocklisted items and avoid repeated duplicate auto-request attempts.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
server/routes/media.ts |
Preserves blocklisted media on deletion by clearing service identifiers instead of removing the entity. |
server/lib/watchlistsync.ts |
Adds pre-filtering to skip blocklisted media and items with existing auto-requests before attempting request creation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…improve item filtering
fallenbagel
changed the title
server/routes/media.ts
Outdated
| media.serviceId = null; | ||
| media.serviceId4k = null; | ||
| media.externalServiceId = null; | ||
| media.externalServiceId4k = null; | ||
| media.externalServiceSlug = null; | ||
| media.externalServiceSlug4k = null; | ||
| media.ratingKey = null; | ||
| media.ratingKey4k = null; | ||
| media.jellyfinMediaId = null; | ||
| media.jellyfinMediaId4k = null; |
There was a problem hiding this comment.
This feels a bit hardcoded. Don't we have another way of resetting values of the entity? Doing this seems to be a good way of having issues later when we'll add other fields to the entity.
There was a problem hiding this comment.
Or delete/recreate the entity with the blacklisted status instead?
There was a problem hiding this comment.
Or delete/recreate the entity with the blacklisted status instead?
The delete/recreate approach won't work because, deleting the Media would kill the Blocklist row before you can recreate.
One thing i could try is dynamically handling non-nullable columns to survive, and nullable ones to get cleared. Wdyt?
for (const column of mediaRepository.metadata.columns) {
if (column.isNullable && !column.isPrimary) {
updatePayload[column.propertyName] = null;
}
}
And then
await mediaRepository.update
There was a problem hiding this comment.
Yep, looks like more future-proof.
There was a problem hiding this comment.
@gauthier-th Actually i just realised that approach wont work:
Nulling all nullable columns also wipes tvdbId, imdbId, and mediaAddedAt which aren't service data but they're media metadata required to identify them. If an admin later un-blocklists the media, those identifiers would be gone.
another approach would then be hardcoding these fields in the entity itself rather than the route. Like resetServiceData() method on the entity. Wdyt? Atleast then everything will be in one place and will be easier to remember to update
There was a problem hiding this comment.
Good point. I do prefer the resetServiceData() option since it keeps all the entity logic at the same place.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }); | ||
|
|
||
| await mediaRepository.remove(media); | ||
| if (media.status === MediaStatus.BLOCKLISTED) { |
There was a problem hiding this comment.
The deletion guard only checks media.status but not media.status4k. According to the Blocklist.addToBlocklist method (server/entity/Blocklist.ts:99-100), both status and status4k are set to BLOCKLISTED when media is added to the blocklist. The check should be media.status === MediaStatus.BLOCKLISTED || media.status4k === MediaStatus.BLOCKLISTED to ensure the blocklist is preserved if either status indicates the media is blocklisted.
| if (media.status === MediaStatus.BLOCKLISTED) { | |
| if ( | |
| media.status === MediaStatus.BLOCKLISTED || | |
| media.status4k === MediaStatus.BLOCKLISTED | |
| ) { |
| const updatePayload: Record<string, null> = {}; | ||
|
|
||
| for (const column of mediaRepository.metadata.columns) { | ||
| if (column.isNullable && !column.isPrimary) { | ||
| updatePayload[column.propertyName] = null; | ||
| } | ||
| } | ||
|
|
||
| await mediaRepository.update(media.id, updatePayload); |
There was a problem hiding this comment.
The current implementation nulls ALL nullable columns, which is overly broad and may have unintended consequences. This includes fields like tvdbId, imdbId, mediaAddedAt, and other metadata fields that should be preserved for blocklisted media. The description mentions "nulling out service-related fields," but the implementation goes beyond that. Consider explicitly specifying which fields to null (e.g., serviceId, serviceId4k, externalServiceId, externalServiceId4k, externalServiceSlug, externalServiceSlug4k, ratingKey, ratingKey4k, jellyfinMediaId, jellyfinMediaId4k) instead of nulling all nullable columns.
| (m) => | ||
| m.tmdbId === i.tmdbId && | ||
| ((m.status !== MediaStatus.UNKNOWN && m.mediaType === 'movie') || | ||
| (m.status === MediaStatus.BLOCKLISTED || |
There was a problem hiding this comment.
The blocklist filtering only checks m.status === MediaStatus.BLOCKLISTED but not m.status4k. According to Blocklist.addToBlocklist (server/entity/Blocklist.ts:99-100), both status and status4k are set to BLOCKLISTED. The check should be m.status === MediaStatus.BLOCKLISTED || m.status4k === MediaStatus.BLOCKLISTED to ensure blocklisted media is properly excluded regardless of which status field indicates it's blocklisted.
| (m.status === MediaStatus.BLOCKLISTED || | |
| (m.status === MediaStatus.BLOCKLISTED || | |
| m.status4k === MediaStatus.BLOCKLISTED || |
2 participants
Description
When a user blocklists media and then removes it from Sonarr/Radarr via the Seerr interface, the Media entity and its associated Blocklist record were both destroyed due to the cascade delete on the relationship. On the next watchlist sync cycle, no record existed to check against, so a fresh request was created and auto-approved, completely bypassing the blocklist.
This PR fixes the media delete endpoint to detect blocklisted status and preserve the entity by nulling out service-related fields rather than removing the row entirely. It also improves the watchlist sync filtering to exclude blocklisted media as a safety net and to skip items where the user already has an existing auto-request, eliminating the repeated unnecessary api calls and database lookups that were happening every sync cycle just to throw a duplicate request error.
How Has This Been Tested?
Screenshots / Logs (if applicable)
Checklist:
pnpm buildpnpm i18n:extractSummary by CodeRabbit
Release Notes