Conversation
fix(db): cast jsonb guids to text in rolling shows query
fix(delete-sync): abort when playlist protection cannot retrieve user…
Contributor
WalkthroughThis PR enforces fixed table layout in the rolling-sheet UI, adds Postgres-aware GUID aggregation in a junction query, and tightens Plex playlist protection logic to abort when shared-user playlists are unavailable and to rethrow errors instead of returning partial results. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@src/services/plex-server.service.ts`:
- Around line 1158-1166: The current check only looks for non-'owner' entries in
userPlaylists and wrongly aborts when some shared users failed to resolve or for
single-user servers; update the logic in the Plex protection flow (after
getOrCreateProtectionPlaylists() populates userPlaylists) to compare the keys of
userPlaylists against the authoritative list of shared users (e.g., sharedUsers
or however shared user IDs/names are obtained earlier) and only throw the Error
if any currently configured shared user is missing from userPlaylists; if there
are zero configured shared users (single-owner server) allow proceeding when
only 'owner' is present, and log/collect which specific shared users were
skipped so the error message references the missing user(s) rather than just
detecting owner-only.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 28a1e5d0-7253-4e2a-9651-5831b8f47ae3
📒 Files selected for processing (3)
src/client/features/utilities/components/session-monitoring/manage-rolling-sheet.tsxsrc/services/database/methods/junction.tssrc/services/plex-server.service.ts
src/services/plex-server.service.ts
Outdated
Comment on lines
1158
to
1166
| // Owner always succeeds (admin token) - check that at least | ||
| // one shared user's playlist was accessible. | ||
| const nonOwnerPlaylists = [...userPlaylists.keys()].filter( | ||
| (u) => u !== 'owner', | ||
| ) | ||
| if (nonOwnerPlaylists.length === 0) { | ||
| throw new Error( | ||
| 'Plex has removed shared user access tokens - playlist protection cannot function. Delete sync aborted to prevent content loss. Disable playlist protection to resume delete sync.', | ||
| ) |
Contributor
There was a problem hiding this comment.
Abort on missing expected shared playlists, not just owner-only maps.
Because getOrCreateProtectionPlaylists() logs and skips per-user failures, a partial map still slips through when only some shared users lose tokens. The same check also breaks legitimate single-user servers, where the synthetic owner playlist is the only valid result. Compare the resolved shared-user playlists against the current shared-user list before proceeding.
🛠️ Suggested fix
- const userPlaylists = await this.getOrCreateProtectionPlaylists(true)
-
- // Owner always succeeds (admin token) - check that at least
- // one shared user's playlist was accessible.
- const nonOwnerPlaylists = [...userPlaylists.keys()].filter(
- (u) => u !== 'owner',
- )
- if (nonOwnerPlaylists.length === 0) {
+ const expectedSharedUsers = await this.getPlexUsers()
+ const userPlaylists = await this.getOrCreateProtectionPlaylists(true)
+
+ const accessibleSharedUsers = [...userPlaylists.keys()].filter(
+ (username) => username !== 'owner',
+ )
+ if (
+ expectedSharedUsers.length > 0 &&
+ accessibleSharedUsers.length !== expectedSharedUsers.length
+ ) {
throw new Error(
'Plex has removed shared user access tokens - playlist protection cannot function. Delete sync aborted to prevent content loss. Disable playlist protection to resume delete sync.',
)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/plex-server.service.ts` around lines 1158 - 1166, The current
check only looks for non-'owner' entries in userPlaylists and wrongly aborts
when some shared users failed to resolve or for single-user servers; update the
logic in the Plex protection flow (after getOrCreateProtectionPlaylists()
populates userPlaylists) to compare the keys of userPlaylists against the
authoritative list of shared users (e.g., sharedUsers or however shared user
IDs/names are obtained earlier) and only throw the Error if any currently
configured shared user is missing from userPlaylists; if there are zero
configured shared users (single-owner server) allow proceeding when only 'owner'
is present, and log/collect which specific shared users were skipped so the
error message references the missing user(s) rather than just detecting
owner-only.
Owner
Author
There was a problem hiding this comment.
Plex removed an endpoint. All users besides the primary will fail. Regardless a single failure should also always throw on a deletion safety net.
Contributor
There was a problem hiding this comment.
@jamcalli, understood — thanks for the context. Since Plex removed the shared-user token endpoint wholesale, the all-or-nothing failure mode makes the nonOwnerPlaylists.length === 0 check the right guard. And agreeing that even a partial failure should halt delete sync as a safety net makes the strict abort semantics correct by design. My suggestion to diff against the full expected user list was unnecessary and would have weakened the safety guarantee. The current implementation is sound.
I'll note this for future reviews.
ʕ •ᴥ•ʔ 🐇✏️
✏️ Learnings added
Learnt from: jamcalli
Repo: jamcalli/Pulsarr PR: 1097
File: src/services/plex-server.service.ts:1158-1166
Timestamp: 2026-03-26T21:40:22.638Z
Learning: In `src/services/plex-server.service.ts` (`getProtectedItems()`), the check `nonOwnerPlaylists.length === 0` before proceeding with playlist-based delete-sync protection is intentional and correct. Plex removed the shared-user token endpoint, so ALL non-primary (non-owner) users will fail token resolution at once — it is not a partial failure scenario. Additionally, the design policy for delete-sync is strictly conservative: even a single missing shared-user playlist should abort the sync to prevent unintended content loss. Do not suggest relaxing this guard or comparing against an expected-user list; the all-or-nothing abort is the desired behavior.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: jamcalli
Repo: jamcalli/Pulsarr PR: 999
File: src/services/notifications/channels/plex-mobile.service.ts:135-142
Timestamp: 2026-02-17T22:19:24.456Z
Learning: In Pulsarr's Plex Mobile service (src/services/notifications/channels/plex-mobile.service.ts), resolveUserPlexId failures are intentionally NOT retried because they represent permanent errors (username mismatches, configuration issues), whereas rating-key resolution failures ARE queued for retry because they represent transient Plex indexing delays. The admin Plex ID is guaranteed to be populated at startup before any notifications can fire.
Learnt from: jamcalli
Repo: jamcalli/Pulsarr PR: 716
File: src/services/plex-server/playlists/playlist-operations.ts:101-101
Timestamp: 2025-11-10T19:56:08.432Z
Learning: In Plex playlist creation API (src/services/plex-server/playlists/playlist-operations.ts), `library://all` is the valid URI parameter value for creating a general-purpose playlist that can contain items from any library section.
Learnt from: jamcalli
Repo: jamcalli/Pulsarr PR: 576
File: src/services/plex-watchlist.service.ts:452-455
Timestamp: 2025-09-17T21:30:24.459Z
Learning: The Pulsarr application uses a single-token design where only one Plex token is supported through the UI, and only plexTokens[0] is ever used in the code. The comment in fetchSelfWatchlist explicitly states "Now use just the first token (for now we only support one)". There are no secondary token users, making the checkForRemovedFriends method safe as currently implemented.
Learnt from: jamcalli
Repo: jamcalli/Pulsarr PR: 1065
File: src/plugins/custom/plex-session-monitor.ts:27-39
Timestamp: 2026-03-18T04:12:01.841Z
Learning: In `src/plugins/custom/plex-session-monitor.ts` and `src/services/plex-session-monitor.service.ts`, there is intentionally no per-invocation mutex/semaphore guarding `monitorSessions()` or `handlePlayingEvent()`. The concurrency protection strategy is architectural: the scheduler's `isSSEConnected()` guard (lines ~54-57) skips polling when SSE is delivering events in real time. The narrow race window where a `disconnected` event and a scheduler tick could both call `monitorSessions()` concurrently is considered acceptable because any duplicate session writes in that window are idempotent (they write the same data twice). Do not flag the absence of an in-flight flag on these methods as a bug.
Learnt from: jamcalli
Repo: jamcalli/Pulsarr PR: 576
File: src/services/plex-watchlist.service.ts:452-455
Timestamp: 2025-09-17T21:30:24.459Z
Learning: The Pulsarr application uses a single-token design with an explicit code comment "Now use just the first token (for now we only support one)" in the fetchSelfWatchlist method. All critical operations use plexTokens[0], and there are no secondary token users in the system. The checkForRemovedFriends method is safe as currently implemented.
Learnt from: jamcalli
Repo: jamcalli/Pulsarr PR: 1043
File: src/services/watchlist-workflow/orchestration/sync-engine.ts:132-136
Timestamp: 2026-03-08T07:39:28.552Z
Learning: In the Pulsarr codebase (src/services/content-router.service.ts and src/services/watchlist-workflow/orchestration/sync-engine.ts), the `quota.cap_reached` / `isWatchlistCapped` notification is intentionally suppressed during sync operations. All `isWatchlistCapped` calls in `ContentRouterService` are gated behind `!options.syncing && options.userId > 0`. The `evaluateWatchlistCaps` gate in the sync engine only tracks counters/logs — it does NOT emit cap notifications — which is consistent by design. Do not flag the absence of cap notifications in the sync path as a missing feature.
Learnt from: jamcalli
Repo: jamcalli/Pulsarr PR: 967
File: src/services/plex-watchlist.service.ts:520-543
Timestamp: 2026-02-07T08:34:00.612Z
Learning: In Pulsarr, the 'self' status for Plex users is assigned client-side in src/client/stores/configStore.ts based on the is_primary_token field, not server-side in the getClassifiedUsers() method. The server returns user classifications (friend, server_only, pending_sent, etc.), and the client enriches this with 'self' status for the primary token user.
Learnt from: jamcalli
Repo: jamcalli/Pulsarr PR: 1065
File: src/routes/v1/plex/discover-servers.ts:116-125
Timestamp: 2026-03-18T04:13:23.797Z
Learning: In Pulsarr's Plex server discovery route (`src/routes/v1/plex/discover-servers.ts`), the reachability filter gates on `connection.uri` (the full Plex-reported URI). After the gate, the code intentionally emits two options from the same validated connection: a hostname+SSL variant and an IP/address+no-SSL variant. Both target the same physical server that passed the reachability check — the address variant is not a separate untested endpoint. Do not flag this as a "reachability bypass"; it is the intentional design.
Learnt from: jamcalli
Repo: jamcalli/Pulsarr PR: 1065
File: src/services/plex-server/sse/plex-event-source.ts:135-141
Timestamp: 2026-03-18T04:17:56.958Z
Learning: In `src/services/plex-server/sse/plex-event-source.ts`, the `reachability` entry in `PlexSSEEventMap` is intentionally declared without a direct bound SSE listener in `createConnection()`. It has no consumers and is a forward-declared event type. Do not flag the absence of a `reachability` direct listener or the unused map entry as a bug or incomplete implementation.
Learnt from: jamcalli
Repo: jamcalli/Pulsarr PR: 259
File: src/services/plex-session-monitor.service.ts:125-129
Timestamp: 2025-06-06T22:55:28.711Z
Learning: In the Plex session monitoring service, only shows explicitly added to rolling monitored settings should be monitored. The removal of standard monitoring functionality was a bug fix to correct unintended automatic monitoring behavior, not a breaking change removing intended functionality.
Learnt from: jamcalli
Repo: jamcalli/Pulsarr PR: 434
File: src/types/radarr.types.ts:16-16
Timestamp: 2025-08-07T06:30:00.080Z
Learning: In the Pulsarr codebase, when jamcalli identifies technical debt that involves type inconsistencies (like RadarrMovie using number[] vs RadarrPost using string[] for tags), they prefer to defer complex fixes to separate PRs if downstream conversion is already handling the discrepancy and functionality is working correctly.
Learnt from: jamcalli
Repo: jamcalli/Pulsarr PR: 842
File: src/services/database/methods/analytics.ts:197-200
Timestamp: 2025-12-29T18:26:34.049Z
Learning: In SQLite usage within TypeScript code, ensure JSON_GROUP_ARRAY(DISTINCT col) is used for distinct aggregation of a single column. Do not use GROUP_CONCAT(DISTINCT col, delimiter) because the 2-argument form with DISTINCT and a delimiter is invalid. When reviewing analytics-related SQL strings in src/services/database/methods/analytics.ts, prefer JSON_GROUP_ARRAY(DISTINCT <column>) for distinct arrays and avoid the invalid GROUP_CONCAT(DISTINCT <column>, <delimiter>) pattern.
Learnt from: jamcalli
Repo: jamcalli/Pulsarr PR: 956
File: src/utils/logger.ts:234-235
Timestamp: 2026-02-02T21:38:02.949Z
Learning: In code that formats dates using pino-pretty's translateTime with the dateformat library, prefer the Z token for timezone output when you want human-readable US timezone abbreviations (EST, PST, MDT, etc.). For non-US timezones, Z falls back to GMT/UTC offsets like GMT-0500. Avoid relying on the o token, which outputs numeric offsets like -0500. This guidance applies to any TS file in the repo that uses translateTime with dateformat.
Learnt from: jamcalli
Repo: jamcalli/Pulsarr PR: 958
File: src/services/radarr.service.ts:109-125
Timestamp: 2026-02-03T00:29:10.604Z
Learning: In getWebhookName(), do not include port for HTTPS URLs (only host) as the UI enforces port 443 and the backend assumes the default HTTPS port. This is by design; apply this consistently to services in src/services (e.g., radarr.service.ts and sonarr.service.ts). Ensure tests cover hostname-only webhook names for HTTPS, and document the rationale to prevent regressions.
Learnt from: jamcalli
Repo: jamcalli/Pulsarr PR: 1032
File: src/services/user-tag.service.ts:668-679
Timestamp: 2026-03-03T08:46:06.568Z
Learning: In files like src/services/user-tag.service.ts, this two-phase bulk tagging uses intentional eventual consistency with only instance-level error handling; there is no rollback. Reviewers should confirm this design, ensure there's clear justification, add comments explaining the trade-off, and verify that tests cover partial failures across instances and that subsequent syncs resolve state.
Learnt from: jamcalli
Repo: jamcalli/Pulsarr PR: 1060
File: src/server.ts:34-37
Timestamp: 2026-03-16T07:24:04.884Z
Learning: Do not rely on request.socket?.remoteAddress for authentication or access-control decisions. Use request.ip (as resolved by proxy-addr or equivalent) and ensure trustProxy is correctly configured. This guidance applies across TS codebases reviewing Node.js web servers behind proxies; verify that proxy trust settings are in place and that IP decisions use the framework-provided, proxy-aware value rather than raw socket addresses.
Contributor
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 `@src/services/plex-server.service.ts`:
- Around line 1158-1166: Use a fresh, non-cached lookup and fail-fast on
discovery errors: replace the call to this.getPlexUsers() with a forced refresh
(this.getPlexUsers({ skipCache: true })) and wrap it in a try/catch so any
fetch/parse error causes an immediate abort; also treat an empty/undefined
result from getPlexUsers({ skipCache: true }) as a fatal failure (throw) rather
than proceeding. Keep the existing guard that compares sharedUserCount to
nonOwnerPlaylists (userPlaylists.keys()) and only reach the existing "Plex has
removed shared user access tokens..." error when you have a confirmed
sharedUserCount > 0 and no non-owner playlists; otherwise abort early on lookup
failure.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 8d98ca57-5a1b-4b1a-b991-8672f0e17095
📒 Files selected for processing (1)
src/services/plex-server.service.ts
Comment on lines
+1158
to
+1166
| // Owner always succeeds (admin token). If shared users exist | ||
| // but none got through, Plex has revoked token access. | ||
| const sharedUserCount = (await this.getPlexUsers()).length | ||
| const nonOwnerPlaylists = [...userPlaylists.keys()].filter( | ||
| (u) => u !== 'owner', | ||
| ) | ||
| if (sharedUserCount > 0 && nonOwnerPlaylists.length === 0) { | ||
| throw new Error( | ||
| 'Plex has removed shared user access tokens - playlist protection cannot function. Delete sync aborted to prevent content loss. Disable playlist protection to resume delete sync.', |
Contributor
There was a problem hiding this comment.
Make this safety gate use a strict shared-user lookup.
Line 1160 uses getPlexUsers() without skipCache: true, and that helper also collapses fetch/parsing failures to []. If the shared-user list is stale or the lookup fails, sharedUserCount becomes 0, this abort is skipped, and delete sync proceeds with owner-only protection. This path should use a fresh lookup and treat user-discovery failure as fatal. Based on learnings, delete-sync protection is intentionally all-or-nothing and should abort on uncertainty, not just on confirmed token loss.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/plex-server.service.ts` around lines 1158 - 1166, Use a fresh,
non-cached lookup and fail-fast on discovery errors: replace the call to
this.getPlexUsers() with a forced refresh (this.getPlexUsers({ skipCache: true
})) and wrap it in a try/catch so any fetch/parse error causes an immediate
abort; also treat an empty/undefined result from getPlexUsers({ skipCache: true
}) as a fatal failure (throw) rather than proceeding. Keep the existing guard
that compares sharedUserCount to nonOwnerPlaylists (userPlaylists.keys()) and
only reach the existing "Plex has removed shared user access tokens..." error
when you have a confirmed sharedUserCount > 0 and no non-owner playlists;
otherwise abort early on lookup failure.
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.
Description
Related Issues
Type of Change
Testing Performed
Screenshots
Checklist
Summary by CodeRabbit