Address PR review: fix event-scope/race bugs, sanitize input, harden exit tracking

- connectivity_check_failed: split check_status() into a pure method and a
  new cron_check_status() wrapper, so interactive connection checks/saves
  (test_connection()) no longer emit it alongside connection_test_result.
- bulk sync: rest_start_sync() now calls mark_run_started() before
  start_queue() (was after, dropping early thread results from the tally),
  and uses shutdown_queue('queue') instead of stop_queue() on manual stop
  so the run state doesn't leak into the next run's sync_completed.
- sync_completed tally: split into two atomically-incremented options
  (RUN_TALLY_SYNCED_KEY/RUN_TALLY_ERRORS_KEY) instead of a single
  get/modify/update array, which could lose updates across concurrent
  sync threads.
- sync_settings_changed: added a diff guard so no-op saves (e.g. the
  wizard re-submitting auto_sync unconditionally) don't fire the event.
- cache_items_viewed: only tracked on page 1 with no search term, instead
  of on every pagination/search request against an open cache point.
- notice_dismissed: sanitize the notice token via sanitize_key() before
  using it as the transient key and event param.
- deactivation_skipped: replaced a 150ms setTimeout guess with a new
  Analytics.trackReliable() using navigator.sendBeacon(), which the
  browser guarantees to dispatch across the immediate navigation.
- Moved a misplaced class constant to the top of Admin with the others.

tests/e2e/sync-analytics.spec.js's cleanup helper updated for the new
split tally option names. Full 32-test e2e suite verified green.

Author: gabriel-detassigny

js/cloudinary.js

@@ -1 +1 @@
-<?php return array('dependencies' => array('wp-api-fetch'), 'version' => 'e3528eb2e86ccc7789df');
+<?php return array('dependencies' => array('wp-api-fetch'), 'version' => '9c563a4d047b892cd84e');

js/deactivate.asset.php

@@ -339,9 +339,14 @@ public function rest_get_caches( $request ) {
 		$current_page = $page ? $page : 1;
 		$data         = $this->get_assets( $parent->ID, $search, $current_page );
 
-		$analytics = get_plugin_instance()->get_component( 'analytics' );
-		if ( $analytics ) {
-			$analytics->track( 'cache_items_viewed', 'cache', null, array( 'cache_point' => (string) $url ) );
+		// This endpoint is also hit for pagination and search-as-you-type
+		// within an already-open cache point; only the initial open (page 1,
+		// no search term) represents a genuine "viewed" event.
+		if ( 1 === (int) $current_page && empty( $search ) ) {
+			$analytics = get_plugin_instance()->get_component( 'analytics' );
+			if ( $analytics ) {
+				$analytics->track( 'cache_items_viewed', 'cache', null, array( 'cache_point' => (string) $url ) );
+			}
 		}
 
 		return rest_ensure_response( $data );

js/deactivate.js

@@ -77,6 +77,18 @@ class Admin {
 	 */
 	const NOTICE_SLUG = '_cld_notices';
 
+	/**
+	 * Settings slugs (by page) that directly inject transformation qualifiers
+	 * (format, quality, freeform string) into delivered URLs, as opposed to
+	 * plain feature-enablement toggles.
+	 *
+	 * @var array
+	 */
+	const GLOBAL_TRANSFORMATION_SLUGS = array(
+		'image_settings' => array( 'image_format', 'image_quality', 'image_freeform' ),
+		'video_settings' => array( 'video_format', 'video_quality', 'video_freeform' ),
+	);
+
 	/**
 	 * Initiate the settings object.
 	 *
@@ -134,14 +146,14 @@ public function rest_endpoints( $endpoints ) {
 	 * @param WP_REST_Request $request The request object.
 	 */
 	public function rest_dismiss_notice( WP_REST_Request $request ) {
-		$token    = $request->get_param( 'token' );
+		$token    = sanitize_key( $request->get_param( 'token' ) );
 		$duration = $request->get_param( 'duration' );
 
 		set_transient( $token, true, $duration );
 
 		$analytics = $this->plugin->get_component( 'analytics' );
 		if ( $analytics ) {
-			$analytics->track( 'notice_dismissed', 'settings', null, array( 'notice_id' => (string) $token ) );
+			$analytics->track( 'notice_dismissed', 'settings', null, array( 'notice_id' => $token ) );
 		}
 	}
 
@@ -495,18 +507,6 @@ protected function save_settings( $submission, $data ) {
 		do_action( 'cloudinary_flush_cache' );
 	}
 
-	/**
-	 * Settings slugs (by page) that directly inject transformation qualifiers
-	 * (format, quality, freeform string) into delivered URLs, as opposed to
-	 * plain feature-enablement toggles.
-	 *
-	 * @var array
-	 */
-	const GLOBAL_TRANSFORMATION_SLUGS = array(
-		'image_settings' => array( 'image_format', 'image_quality', 'image_freeform' ),
-		'video_settings' => array( 'video_format', 'video_quality', 'video_freeform' ),
-	);
-
 	/**
 	 * Emits `transformation_applied` (scope: global) when a settings save
 	 * changed a global transformation field.

php/assets/class-rest-assets.php

@@ -644,12 +644,30 @@ public function updated_option() {
 	/**
 	 * Check the status of Cloudinary.
 	 *
+	 * Also called interactively from `test_connection()` (wizard live-typing
+	 * checks and connection saves) — `connectivity_check_failed` is NOT
+	 * tracked here so those don't get miscounted as background connectivity
+	 * failures. See `cron_check_status()` for the cron-only variant that does.
+	 *
 	 * @return array|WP_Error
 	 */
 	public function check_status() {
 		$status = $this->test_ping();
 		$this->settings->get_setting( 'status' )->save_value( $status );
 
+		return $status;
+	}
+
+	/**
+	 * Cron-only wrapper around `check_status()` that tracks connectivity
+	 * failures. Kept separate from `check_status()` so interactive callers
+	 * (the wizard's live check, connection saves) never emit this event.
+	 *
+	 * @return array|WP_Error
+	 */
+	public function cron_check_status() {
+		$status = $this->check_status();
+
 		if ( is_wp_error( $status ) ) {
 			$analytics = $this->plugin->get_component( 'analytics' );
 			if ( $analytics ) {
@@ -825,7 +843,7 @@ public function setup() {
 	 * Setup Status cron.
 	 */
 	protected function setup_status_cron() {
-		Cron::register_process( 'check_status', array( $this, 'check_status' ), DAY_IN_SECONDS );
+		Cron::register_process( 'check_status', array( $this, 'cron_check_status' ), DAY_IN_SECONDS );
 	}
 
 	/**

php/class-admin.php

@@ -180,13 +180,22 @@ public function rest_start_sync( \WP_REST_Request $request ) {
 			'success' => false,
 		);
 		if ( empty( $start ) ) {
-			$this->queue->stop_queue();
+			// A real, user-driven stop (the bulk-sync toggle was switched off)
+			// rather than `stop_maybe()`'s internal restart-to-catch-new-items
+			// cycle, so this needs to actually close out the tracked run —
+			// `shutdown_queue()` does that via `track_run_completed()`, unlike
+			// the bare `stop_queue()` that cycle uses.
+			$this->queue->shutdown_queue( 'queue' );
 		} else {
+			// Set before starting the queue: `start_queue()` immediately spawns
+			// background threads whose sync results would otherwise be tallied
+			// against a run that doesn't exist yet and get silently dropped. On
+			// a failed start, `start_queue()` itself calls `shutdown_queue()`,
+			// which cleans up the run state this just staged.
+			$this->queue->mark_run_started( $type );
 			$state['success'] = $this->queue->start_queue( $type );
 
 			if ( $state['success'] ) {
-				$this->queue->mark_run_started( $type );
-
 				$analytics = $this->plugin->get_component( 'analytics' );
 				if ( $analytics ) {
 					$queue = $this->queue->get_queue( $type );

php/class-connect.php

@@ -62,11 +62,24 @@ class Sync_Queue {
 	const RUN_STARTED_KEY = '_cloudinary_sync_run_started';
 
 	/**
-	 * Option key for the running success/error tally of the current run.
+	 * Option key for the running count of successful syncs in the current run.
+	 *
+	 * Kept as its own scalar option (rather than a field in a single
+	 * serialized array) so it can be incremented atomically — the queue runs
+	 * multiple concurrent threads as separate requests, and a
+	 * get_option()/update_option() read-modify-write round trip on a shared
+	 * array would lose updates under that concurrency.
 	 *
 	 * @var     string
 	 */
-	const RUN_TALLY_KEY = '_cloudinary_sync_run_tally';
+	const RUN_TALLY_SYNCED_KEY = '_cloudinary_sync_run_tally_synced';
+
+	/**
+	 * Option key for the running count of failed syncs in the current run.
+	 *
+	 * @var     string
+	 */
+	const RUN_TALLY_ERRORS_KEY = '_cloudinary_sync_run_tally_errors';
 
 	/**
 	 * The cron frequency to ensure that the queue is progressing.
@@ -226,14 +239,8 @@ public function mark_run_started( $type = 'queue' ) {
 			return;
 		}
 		update_option( self::RUN_STARTED_KEY, time(), false );
-		update_option(
-			self::RUN_TALLY_KEY,
-			array(
-				'synced' => 0,
-				'errors' => 0,
-			),
-			false
-		);
+		delete_option( self::RUN_TALLY_SYNCED_KEY );
+		delete_option( self::RUN_TALLY_ERRORS_KEY );
 	}
 
 	/**
@@ -249,17 +256,27 @@ public function tally_run_result( $success ) {
 		if ( false === get_option( self::RUN_STARTED_KEY, false ) ) {
 			return;
 		}
-		$tally = get_option(
-			self::RUN_TALLY_KEY,
-			array(
-				'synced' => 0,
-				'errors' => 0,
+		$this->increment_option( $success ? self::RUN_TALLY_SYNCED_KEY : self::RUN_TALLY_ERRORS_KEY );
+	}
+
+	/**
+	 * Atomically increments an integer option, creating it as 1 if missing.
+	 *
+	 * @param string $option_name The option to increment.
+	 *
+	 * @return void
+	 */
+	protected function increment_option( $option_name ) {
+		global $wpdb;
+
+		$wpdb->query( // phpcs:ignore WordPress.DB
+			$wpdb->prepare(
+				"INSERT INTO {$wpdb->options} (option_name, option_value, autoload) VALUES (%s, '1', 'no')
+				 ON DUPLICATE KEY UPDATE option_value = option_value + 1",
+				$option_name
 			)
 		);
-		$key   = $success ? 'synced' : 'errors';
-
-		$tally[ $key ] = isset( $tally[ $key ] ) ? $tally[ $key ] + 1 : 1;
-		update_option( self::RUN_TALLY_KEY, $tally, false );
+		wp_cache_delete( $option_name, 'options' );
 	}
 
 	/**
@@ -274,15 +291,11 @@ protected function track_run_completed() {
 		if ( false === $started ) {
 			return;
 		}
-		$tally = get_option(
-			self::RUN_TALLY_KEY,
-			array(
-				'synced' => 0,
-				'errors' => 0,
-			)
-		);
+		$synced = (int) get_option( self::RUN_TALLY_SYNCED_KEY, 0 );
+		$errors = (int) get_option( self::RUN_TALLY_ERRORS_KEY, 0 );
 		delete_option( self::RUN_STARTED_KEY );
-		delete_option( self::RUN_TALLY_KEY );
+		delete_option( self::RUN_TALLY_SYNCED_KEY );
+		delete_option( self::RUN_TALLY_ERRORS_KEY );
 
 		$analytics = $this->plugin->get_component( 'analytics' );
 		if ( $analytics ) {
@@ -291,8 +304,8 @@ protected function track_run_completed() {
 				'sync',
 				null,
 				array(
-					'total_synced' => isset( $tally['synced'] ) ? (int) $tally['synced'] : 0,
-					'total_errors' => isset( $tally['errors'] ) ? (int) $tally['errors'] : 0,
+					'total_synced' => $synced,
+					'total_errors' => $errors,
 					'duration_sec' => time() - (int) $started,
 				)
 			);
@@ -330,19 +343,33 @@ public function track_setting_changed( $new_value, $current_value, $setting ) {
 		$parts     = explode( '.', $full_slug );
 		$slug      = end( $parts );
 
-		if ( in_array( $slug, $tracked_slugs, true ) ) {
-			$analytics = $this->plugin->get_component( 'analytics' );
-			if ( $analytics ) {
-				$analytics->track(
-					'sync_settings_changed',
-					'sync',
-					null,
-					array(
-						'setting_key' => $slug,
-						'new_value'   => $new_value,
-					)
-				);
-			}
+		if ( ! in_array( $slug, $tracked_slugs, true ) ) {
+			return $new_value;
+		}
+
+		// Not every `set_pending()` caller passes $current_value (e.g. the
+		// wizard's save re-submits `auto_sync` unconditionally); fall back to
+		// the setting's own current value, which `@data` still reflects here
+		// since `set_pending()` only stages the pending write.
+		if ( null === $current_value ) {
+			$current_value = $setting->get_value();
+		}
+
+		if ( $new_value === $current_value ) {
+			return $new_value;
+		}
+
+		$analytics = $this->plugin->get_component( 'analytics' );
+		if ( $analytics ) {
+			$analytics->track(
+				'sync_settings_changed',
+				'sync',
+				null,
+				array(
+					'setting_key' => $slug,
+					'new_value'   => $new_value,
+				)
+			);
 		}
 
 		return $new_value;
@@ -699,7 +726,7 @@ public function stop_maybe( $type = 'queue' ) {
 	 *
 	 * @param string $type The type of queue to shutdown.
 	 */
-	protected function shutdown_queue( $type = 'queue' ) {
+	public function shutdown_queue( $type = 'queue' ) {
 		if ( 'queue' === $type ) {
 			delete_option( self::$queue_enabled );
 			$this->track_run_completed();