Conversation
#838 Replace delete_option() with race-safe reconciliation that re-reads the option from DB and only removes processed users whose data hasn't changed, preserving entries written by concurrent requests (Scenario E). Co-Authored-By: Claude via AIContext
Co-Authored-By: Claude via AIContext
The delete handler only cleared the URL field, leaving the attachment ID intact so the player kept showing stale artwork. Co-Authored-By: Claude via AIContext
sync_membership() now guards against null membership lookups. process_single_sync() clears the queue option at the retry cap. Toggle checkbox is now properly associated with its label. Co-Authored-By: Claude via AIContext
This reverts commit fa4ff07.
Co-Authored-By: Claude via AIContext
|
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 "default podcast" toggle with styles and template, improves Castos player accessibility and i18n, refactors WooCommerce single-sync queuing/processing with reconciliation, uses featured image as album-art fallback, adjusts admin notices/Castos connect flow, updates related JS/PHP/templates/tests, and bumps plugin version to 3.16.0-alpha. ChangesDefault Podcast Toggle
Player Accessibility & i18n
WooCommerce Memberships Single-Sync Refactor
Episode Album Art & Tests
Admin Notices & Castos Connect
Miscellaneous / Release
Sequence DiagramsequenceDiagram
participant Admin as Admin User
participant SeriesUI as Series Editor
participant SeriesCtrl as Series Controller
participant Store as Options Store
participant PlayerCtrl as Players Controller
participant PlayerJS as Player JavaScript
participant Player as Castos Player
participant WCInt as WC Memberships Integrator
participant Cron as WP Cron
Admin->>SeriesUI: Toggle "Set as default podcast"
SeriesUI->>SeriesCtrl: POST ssp_default_podcast
SeriesCtrl->>SeriesCtrl: maybe_set_default_series()
SeriesCtrl->>Store: update_option('default_series', term_id)
Store-->>SeriesCtrl: saved
PlayerCtrl->>PlayerJS: wp_localize_script -> provides i18n
Player->>PlayerJS: open share panel
PlayerJS->>PlayerJS: togglePanel(panel, triggerBtn) -> set aria-expanded
Player->>PlayerJS: copy link
PlayerJS->>PlayerJS: copyLink() -> announceToScreenReader("Copied")
WCInt->>WCInt: sync_membership(membership)
WCInt->>WCInt: prepare_single_sync() (write queue)
Cron->>WCInt: process_single_sync()
WCInt->>WCInt: snapshot option, process users, reconcile_sync_data()
WCInt->>Cron: reschedule if pending
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Adds ARIA attributes for screen reader support: aria-pressed on mute toggle, dynamic aria-label on speed button, aria-expanded on panel triggers, Escape to close panels, aria-selected on playlist items, and aria-live announcement for copy actions. Close buttons changed from div to button elements. Co-Authored-By: Claude via AIContext
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@assets/js/castos-player.js`:
- Around line 258-271: In togglePanel, when closing a panel (the branch where
panel.classList.contains('open') is true), after setting
triggerBtn.setAttribute('aria-expanded','false') move focus back to the
triggerBtn so the hidden close control is not left focused (i.e., call focus()
on triggerBtn when it exists) before returning; keep the existing behavior when
opening (panel.focus()) unchanged and ensure you only call triggerBtn.focus() if
triggerBtn is provided.
- Around line 273-293: The copyLink function currently always calls
announceToScreenReader('Copied') regardless of whether
document.execCommand('Copy') succeeded; change copyLink to capture the return
value (or catch exceptions) from document.execCommand('copy') and only call
announceToScreenReader(i18n.copied || 'Copied') when the command returns true
(or no error thrown). Keep announceToScreenReader as-is for updating the live
region, but gate its invocation in copyLink so screen readers are only notified
on successful copy; reference the copyLink and announceToScreenReader functions
when making this change.
In `@php/classes/controllers/class-series-controller.php`:
- Around line 354-379: The handler-level caching of the default series ID causes
stale reads: after maybe_set_default_series() calls ssp_update_option() the
subsequent save_series_data_to_castos() still uses the old cached value from
Series_Handler::default_series_id() (and similar caches in Settings_Handler and
Series_Walker). Fix by invalidating or refreshing the cached value immediately
after updating the option—e.g., after ssp_update_option( 'default_series', (int)
$term_id ) in maybe_set_default_series() clear the handler cache (unset
$this->default_series_id or call a refresh method) or make
save_series_data_to_castos() fetch the live value (call
ssp_get_default_series_id() directly or provide a getter that bypasses the
cached property) so the updated default is used when syncing to Castos.
In `@php/classes/integrations/woocommerce/class-wc-memberships-integrator.php`:
- Line 380: The call to sync_membership() should be guarded because
get_user_membership() can return null; modify the code around the
get_user_membership() call so you assign $user_membership =
$this->get_user_membership(...) then check if $user_membership is truthy (not
null) before invoking $this->sync_membership($user_membership), and skip or
handle the null case (e.g., return or log) to avoid dereferencing a null
membership.
In `@templates/players/castos-player.php`:
- Around line 190-194: The playlist items are focusable but not
keyboard-activatable; update the template element rendering (playlist__item with
data-episode) and the dynamic item creation in assets/js/castos-player.js to
support keyboard activation: either change each list item to a native <button>
element (preserving data-episode and active/aria-selected state) or add a
keydown listener to the playlist__item elements that triggers the same selection
logic on Enter (key === "Enter") and Space (key === " " or key === "Spacebar")
and prevents default on Space; ensure the handler mirrors the existing click
handler and also sets aria-selected and active class consistently for both
static and appended items.
In `@tests/WPUnit/WCMembershipsIntegratorTest.php`:
- Around line 9-12: Add the same scheduled-hook clearing that you have in
tearDown() to setUp() so pre-existing scheduled single-sync state cannot leak
into the first test; in setUp() (alongside
delete_option(WC_Memberships_Integrator::SINGLE_SYNC_DATA_OPTION)) call the same
wp_clear_scheduled_hook or equivalent routine used in tearDown() to unschedule
the single-sync hook for full test isolation.
🪄 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.yaml
Review profile: CHILL
Plan: Pro
Run ID: bc48a33d-bd05-4fca-b5d8-f596a0034a27
📒 Files selected for processing (13)
assets/admin/css/admin.cssassets/admin/scss/_admin.scssassets/js/admin.jsassets/js/castos-player.jsphp/classes/controllers/class-players-controller.phpphp/classes/controllers/class-series-controller.phpphp/classes/integrations/class-abstract-integrator.phpphp/classes/integrations/woocommerce/class-wc-memberships-integrator.phpseriously-simple-podcasting.phptemplates/players/castos-player.phptemplates/settings/podcast-default-toggle.phptests/WPUnit/PlayersControllerTest.phptests/WPUnit/WCMembershipsIntegratorTest.php
sync_membership() guards against null membership lookups. process_single_sync() clears stale queue at retry cap instead of silently abandoning stranded users. Co-Authored-By: Claude via AIContext
Reuse reconcile_sync_data() to drop only capped snapshot users instead of unconditionally deleting the queue option. Co-Authored-By: Claude via AIContext
Falls back to the post's featured image in get_album_art() after cover/series/feed images and before the placeholder. Skips the squareness check since the HTML5 player handles non-square via CSS. Co-Authored-By: Claude via AIContext
Co-Authored-By: Claude via AIContext
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@php/classes/repositories/class-episode-repository.php`:
- Around line 863-867: The code in class-episode-repository.php reads
$image_data_array['src'] directly after calling ssp_get_attachment_image_src()
which can return an empty array and causes an undefined offset warning; update
the check to guard the payload before accessing ['src']—for example, use the
same pattern as other call sites by using ssp_is_image_square($image_data_array)
or at minimum verify isset($image_data_array['src']) && !
empty($image_data_array['src']) before returning apply_filters('ssp_album_art',
...); change the block around $thumb_id / $image_data_array to perform that safe
check.
🪄 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.yaml
Review profile: CHILL
Plan: Pro
Run ID: 44d21368-6d1e-4a5c-a106-3998644a91d3
📒 Files selected for processing (2)
php/classes/repositories/class-episode-repository.phptests/WPUnit/EpisodeRepositoryAlbumArtTest.php
Disconnect notice was persisting after successful reconnection because nothing removed it from the constant notices option. Co-Authored-By: Claude via AIContext
Prevents unsanitized strings from rendering in HTML data attributes. Co-Authored-By: Claude via AIContext
Enable passthrough URLs for podcasts with active campaigns, not just ads.
Castos can push status changes via PUT /ssp/v1/podcasts/{series_id} for
immediate activation; hourly cron sync serves as backup.
Co-Authored-By: Claude via AIContext
Co-Authored-By: Claude via AIContext
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
php/classes/rest/class-episodes-rest-controller.php (1)
324-346: 💤 Low valueConsider batching transient deletions for large sites.
The
flush_episode_file_data_transients()method queries all episodes withposts_per_page => -1, which could cause memory pressure on sites with thousands of episodes. While this is called infrequently (only when passthrough settings change), consider batching:♻️ Suggested improvement
protected function flush_episode_file_data_transients( $series_id ) { $args = array( 'post_type' => ssp_post_types(), 'post_status' => 'any', - 'posts_per_page' => -1, + 'posts_per_page' => 500, 'fields' => 'ids', ); if ( $series_id ) { $args['tax_query'] = array( array( 'taxonomy' => ssp_series_taxonomy(), 'field' => 'term_id', 'terms' => $series_id, ), ); } - $episode_ids = get_posts( $args ); + $paged = 1; + do { + $args['paged'] = $paged; + $episode_ids = get_posts( $args ); - foreach ( $episode_ids as $episode_id ) { - delete_transient( 'ssp_episode_file_data_' . $episode_id ); - } + foreach ( $episode_ids as $episode_id ) { + delete_transient( 'ssp_episode_file_data_' . $episode_id ); + } + $paged++; + } while ( count( $episode_ids ) === 500 ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@php/classes/rest/class-episodes-rest-controller.php` around lines 324 - 346, flush_episode_file_data_transients currently calls get_posts with posts_per_page => -1 which can OOM on large sites; change it to iterate in batches (e.g., set posts_per_page to a reasonable batch size like 100 and use a paged loop or WP_Query with 'fields'=>'ids' and 'paged' to fetch each page) and call delete_transient('ssp_episode_file_data_' . $episode_id) for each ID as you stream through results; ensure you reset query state (wp_reset_postdata) if using WP_Query and stop looping when no more posts are returned to avoid loading all episode IDs into memory at once.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@php/classes/controllers/class-passthrough-controller.php`:
- Around line 107-121: The get_episode_file_data() method currently caches any
response from castos_handler->get_episode_file_data into the transient
'ssp_episode_file_data_{episode_id}' for WEEK_IN_SECONDS; change this so failed
API responses are not cached (or are cached with a much shorter TTL): after
calling $this->castos_handler->get_episode_file_data( $castos_episode_id )
inspect the returned $file_data and if it's falsy or if
isset($file_data->success) && $file_data->success === false then either skip
set_transient entirely or call set_transient with a short TTL (e.g.
MINUTE_IN_SECONDS * 5) instead of WEEK_IN_SECONDS; update the logic in
get_episode_file_data() accordingly so only successful responses get the long
cache.
---
Nitpick comments:
In `@php/classes/rest/class-episodes-rest-controller.php`:
- Around line 324-346: flush_episode_file_data_transients currently calls
get_posts with posts_per_page => -1 which can OOM on large sites; change it to
iterate in batches (e.g., set posts_per_page to a reasonable batch size like 100
and use a paged loop or WP_Query with 'fields'=>'ids' and 'paged' to fetch each
page) and call delete_transient('ssp_episode_file_data_' . $episode_id) for each
ID as you stream through results; ensure you reset query state
(wp_reset_postdata) if using WP_Query and stop looping when no more posts are
returned to avoid loading all episode IDs into memory at once.
🪄 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.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0220165d-3487-4939-8c9c-332c81e1d5f2
📒 Files selected for processing (10)
php/classes/controllers/class-ads-controller.phpphp/classes/controllers/class-app-controller.phpphp/classes/controllers/class-cron-controller.phpphp/classes/controllers/class-passthrough-controller.phpphp/classes/entities/class-api-podcast.phpphp/classes/entities/class-episode-file-data.phpphp/classes/rest/class-episodes-rest-controller.phpphp/classes/rest/class-rest-api-controller.phpphp/includes/ssp-functions.phptests/WPUnit/SSPEpisodeFunctionsTest.php
💤 Files with no reviewable changes (1)
- php/classes/controllers/class-ads-controller.php
✅ Files skipped from review due to trivial changes (2)
- php/classes/rest/class-rest-api-controller.php
- php/classes/entities/class-api-podcast.php
RSS 2.0 only allows <image> at the channel level. The bare tag in feed items broke W3C validation and Spotify ingestion. Co-Authored-By: Claude via AIContext
Co-Authored-By: Claude via AIContext
Co-Authored-By: Claude via AIContext
Restore focus to trigger button on panel close, gate copy announcement on success, add keyboard activation for playlist items, clear scheduled hook in test setUp, and use short TTL for failed API responses. Co-Authored-By: Claude via AIContext
Bumps stable tag and package.json version, adds changelog covering campaigns support, a11y improvements, feed fixes, and WC sync updates. Co-Authored-By: Claude via AIContext
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests