feat(medium): Fix Spotify volume control issues and address code review feedback

app/client/control/components/SpotifyControls.tsx

@@ -11,8 +11,15 @@ import MenuItem from '@mui/material/MenuItem'
 import Select from '@mui/material/Select'
 import Typography from '@mui/material/Typography'
 import { useRouter } from 'next/navigation'
-import { useCallback, useEffect, useMemo, useRef, useReducer } from 'react'
-import { clampVolume } from '@/hooks/useVolumePreference'
+import {
+  useCallback,
+  useEffect,
+  useMemo,
+  useRef,
+  useReducer,
+  useState,
+} from 'react'
+import { clampVolume } from '@/utils/audioManager'
 import { useAppSnackbar } from '@/hooks/useAppSnackbar'
 import { useWebSocket } from '@/context/WebSocketContext'
 import { useSpotifyCommand } from '@/hooks/useSpotifyCommand'
@@ -28,27 +35,22 @@ import { SPOTIFY_BRAND_COLOR } from '@/constants/spotify'
 
 const VOLUME_SLIDER_SX = { mt: 3, mb: 1 }
 
-// 1. State Shape
 interface SpotifyControlsState {
   displayVolume: number
   isMuted: boolean
   isSliding: boolean
   lastVolume: number // Last non-zero volume
-  selectedDeviceId: string
 }
 
-// 2. Actions
 type SpotifyControlsAction =
   | { type: 'SET_VOLUME'; payload: number }
   | { type: 'SET_SLIDING'; payload: boolean }
   | { type: 'TOGGLE_MUTE' }
-  | { type: 'SELECT_DEVICE'; payload: string }
   | {
       type: 'SYNC_WITH_WEBSOCKET'
       payload: { volume?: number; isMuted?: boolean }
     }
 
-// 3. Reducer Logic
 const spotifyControlsReducer = (
   state: SpotifyControlsState,
   action: SpotifyControlsAction
@@ -78,26 +80,19 @@ const spotifyControlsReducer = (
     case 'TOGGLE_MUTE': {
       const newMutedState = !state.isMuted
       if (newMutedState) {
-        // Muting: set volume to 0
         return {
           ...state,
           isMuted: true,
           displayVolume: 0,
         }
       } else {
-        // Unmuting: restore to last known volume
         return {
           ...state,
           isMuted: false,
           displayVolume: state.lastVolume > 0 ? state.lastVolume : 50, // fallback
         }
       }
     }
-    case 'SELECT_DEVICE':
-      return {
-        ...state,
-        selectedDeviceId: action.payload,
-      }
     default:
       return state
   }
@@ -111,7 +106,6 @@ const SpotifyControls = () => {
   const { devices = [] } = spotifyData // Default to empty array if undefined
   const { showWarning } = useAppSnackbar()
 
-  // 4. Integrate useReducer
   const [state, dispatch] = useReducer(spotifyControlsReducer, {
     displayVolume: spotifyData.playback.volume_percent ?? 70,
     isMuted: spotifyData.playback.isMuted ?? false,
@@ -121,15 +115,16 @@ const SpotifyControls = () => {
       spotifyData.playback.volume_percent > 0
         ? spotifyData.playback.volume_percent
         : 70,
-    selectedDeviceId: '',
   })
-  const { displayVolume, isMuted, isSliding, selectedDeviceId } = state
+  const { displayVolume, isMuted, isSliding } = state
+  const [selectedDeviceId, setSelectedDeviceId] = useState<string>('')
 
   const lastSentVolumeRef = useRef<string | null>(null)
   const lastWarningTimeRef = useRef<number>(0)
   const prevActiveIdRef = useRef<string | undefined>(undefined)
-  const lastVolumeSyncTimeRef = useRef<number>(0)
-  const hasPendingSendRef = useRef<boolean>(false)
+  const pendingSendTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(
+    null
+  )
 
   const hrmDevice = useMemo(
     () =>
@@ -166,31 +161,11 @@ const SpotifyControls = () => {
     }
   }, [connectionStatus, sendData, spotifyServiceInitialized])
 
-  // Synchronize with WebSocket data whenever it changes.
-  // We rely on the server as the source of truth for volume, but use a grace period
-  // to prevent local sliders from "jumping" while the user is actively adjusting them.
   useEffect(() => {
-    const timeSinceLastVolumeSend = Date.now() - lastVolumeSyncTimeRef.current
-
-    // Only apply grace period if a send is pending and within the window.
-    // The server broadcasts a SPOTIFY_UPDATE immediately after a SET_VOLUME command,
-    // confirming the new state to all clients.
-    const shouldRespectGracePeriod =
-      hasPendingSendRef.current &&
-      timeSinceLastVolumeSend < VOLUME_SYNC_GRACE_PERIOD_MS
-
-    if (isSliding || shouldRespectGracePeriod) {
+    if (isSliding || pendingSendTimeoutRef.current) {
       return
     }
 
-    // Once grace period has elapsed, clear the pending send flag
-    if (
-      hasPendingSendRef.current &&
-      timeSinceLastVolumeSend >= VOLUME_SYNC_GRACE_PERIOD_MS
-    ) {
-      hasPendingSendRef.current = false
-    }
-
     dispatch({
       type: 'SYNC_WITH_WEBSOCKET',
       payload: {
@@ -207,9 +182,6 @@ const SpotifyControls = () => {
   // Effect to auto-select the active device or HRM Web Player
   useEffect(() => {
     if (devices.length === 0) {
-      if (selectedDeviceId !== '') {
-        dispatch({ type: 'SELECT_DEVICE', payload: '' })
-      }
       return
     }
 
@@ -220,21 +192,23 @@ const SpotifyControls = () => {
       activeDevice &&
       (!selectedDeviceId || activeDevice.id !== prevActiveIdRef.current)
     ) {
-      dispatch({ type: 'SELECT_DEVICE', payload: activeDevice.id })
+      // eslint-disable-next-line react-hooks/set-state-in-effect
+      setSelectedDeviceId(activeDevice.id)
       prevActiveIdRef.current = activeDevice.id
       return
     }
 
     // 2. Selected device no longer exists
     if (selectedDeviceId && !devices.some((d) => d.id === selectedDeviceId)) {
       const nextId = activeDevice?.id || hrmDevice?.id || ''
-      dispatch({ type: 'SELECT_DEVICE', payload: nextId })
+
+      setSelectedDeviceId(nextId)
       return
     }
 
     // 3. Auto-select HRM Web Player if no active device and nothing selected
     if (!selectedDeviceId && !activeDevice && hrmDevice) {
-      dispatch({ type: 'SELECT_DEVICE', payload: hrmDevice.id })
+      setSelectedDeviceId(hrmDevice.id)
     }
   }, [devices, selectedDeviceId, hrmDevice])
 
@@ -320,8 +294,13 @@ const SpotifyControls = () => {
       const messageKey = `${targetDeviceId}:${sanitized}`
       if (lastSentVolumeRef.current === messageKey) return
 
-      hasPendingSendRef.current = true
-      lastVolumeSyncTimeRef.current = Date.now()
+      if (pendingSendTimeoutRef.current) {
+        clearTimeout(pendingSendTimeoutRef.current)
+      }
+
+      pendingSendTimeoutRef.current = setTimeout(() => {
+        pendingSendTimeoutRef.current = null
+      }, VOLUME_SYNC_GRACE_PERIOD_MS)
 
       executeSpotify('SET_VOLUME', {
         volume: sanitized,
@@ -342,15 +321,14 @@ const SpotifyControls = () => {
   )
 
   const handleToggleMute = useCallback(() => {
-    // Calculate the next state to determine the command payload
     const newMutedState = !isMuted
     const newVolume = newMutedState
       ? 0
       : state.lastVolume > 0
         ? state.lastVolume
         : 50
 
-    dispatch({ type: 'TOGGLE_MUTE' }) // Update UI
+    dispatch({ type: 'TOGGLE_MUTE' })
     sendVolumeCommand(newVolume) // Send command with the new volume
   }, [isMuted, state.lastVolume, sendVolumeCommand])
 
@@ -360,6 +338,14 @@ const SpotifyControls = () => {
     }
   }, [connectionStatus])
 
+  useEffect(() => {
+    return () => {
+      if (pendingSendTimeoutRef.current) {
+        clearTimeout(pendingSendTimeoutRef.current)
+      }
+    }
+  }, [])
+
   return (
     <ControlCard
       data-testid="spotify-controls"
@@ -443,7 +429,7 @@ const SpotifyControls = () => {
                     value={selectedDeviceId}
                     onChange={(e) => {
                       const deviceId = e.target.value as string
-                      dispatch({ type: 'SELECT_DEVICE', payload: deviceId })
+                      setSelectedDeviceId(deviceId)
                       if (deviceId) {
                         sendSpotifyCommand('TRANSFER_PLAYBACK', deviceId)
                       }

components/SpotifyDisplay.tsx

@@ -3,7 +3,7 @@
 import { useSpotifyAuth } from '@/hooks/useSpotifyAuth'
 import useSpotifyWebPlayback from '@/hooks/useSpotifyWebPlayback'
 import { useDashboardRegistration } from '@/hooks/useDashboardRegistration'
-import { clampVolume } from '@/hooks/useVolumePreference'
+import { clampVolume } from '@/utils/audioManager'
 import { useWebSocket } from '@/context/WebSocketContext'
 import { useSpotifyCommand } from '@/hooks/useSpotifyCommand'
 import { VOLUME_SYNC_GRACE_PERIOD_MS } from '@/constants/spotify'

hooks/useVolumePreference.ts

@@ -1,12 +1,9 @@
 import { useCallback, useEffect, useRef, useState } from 'react'
-import { audioManager } from '../utils/audioManager'
+import { audioManager, clampVolume } from '@/utils/audioManager'
 
 const STORAGE_KEY_VOL = 'hrm-preferred-volume' // Stores the user's last chosen volume
 const STORAGE_KEY_MUTE = 'hrm-muted'
 
-export const clampVolume = (value: number): number =>
-  Math.min(100, Math.max(0, Math.round(value)))
-
 // Manages user's volume and mute preferences with localStorage persistence.
 const useVolumePreference = (defaultVolume = 70) => {
   const sanitizedDefault = clampVolume(defaultVolume)

tests/playwright/lib/masks.ts

@@ -19,7 +19,7 @@ export const VRT_MASK_SELECTORS = {
   bpmValue: '[data-testid="bpm-value"]',
   caloriesValue: '[data-testid="calories-value"]',
   timerCountdown: '[data-testid="timer-countdown"]',
-  timerPhaseLabel: '[data-testid="timer-phase-label"]',
+  timerPhaseLabel: '[data-testid="timer-phase"]',
   hrTimeSeriesChart: '[data-testid="hr-time-series-chart"]',
   spotifyCurrentTrack: '[data-testid="spotify-current-track-name"]',
 } as const

tests/playwright/vrt-components.spec.ts

@@ -7,6 +7,7 @@ import {
   resetServerState,
   getSpotifyMasks,
 } from './lib'
+import { checkAccessibility } from './lib/accessibility'
 import { takeScreenshot } from './lib/visual'
 import { waitForPageReady } from './lib/waits'
 import { VRT_TIMEOUTS } from './lib/timeouts'
@@ -147,6 +148,9 @@ test.describe('Component-Specific VRT', () => {
     // Wait for the opacity transition to finish rendering
     await expect(menu).toHaveCSS('opacity', '1')
 
+    // Perform manual accessibility check on the specific menu element to ensure context validity
+    await checkAccessibility(dashboardPage)
+
     await takeScreenshot(menu, 'spotify-device-selector-menu.png', {
       threshold: 0.2, // Tighter threshold for the Paper element
       skipA11y: true, // Accessibility checked manually above

tests/playwright/vrt-dashboard.spec.ts

@@ -139,13 +139,13 @@ test.describe('Visual Regression Tests', () => {
         .locator('[data-testid="dashboard"] > div')
         .first()
       await assertFixedDimensions(topRow, {
-        maxHeight: 600, // Increased from 400 to accommodate layout fluctuations
+        maxHeight: 600,
       })
 
       const dashboard = dashboardPage.getByTestId('dashboard')
       await takeScreenshot(dashboard, 'dashboard-active-timer-with-hr.png', {
         mask: [...getDynamicContentMasks(dashboardPage)],
-        maxDiffPixelRatio: 0.15, // Higher threshold for complex combined state
+        maxDiffPixelRatio: 0.15,
       })
     })
 
@@ -173,7 +173,7 @@ test.describe('Visual Regression Tests', () => {
       const dashboard = dashboardPage.getByTestId('dashboard')
       await takeScreenshot(dashboard, 'dashboard-large-desktop.png', {
         mask: getDynamicContentMasks(dashboardPage),
-        maxDiffPixelRatio: 0.25, // Relaxed to handle CI rendering differences
+        maxDiffPixelRatio: 0.15,
       })
     })
   })

tests/playwright/vrt-timer-controls.spec.ts

@@ -72,7 +72,6 @@ test.describe('Visual Regression Tests', () => {
       const timerControls = controlPage.getByTestId('timer-controls')
       await takeScreenshot(timerControls, 'timer-controls-active.png', {
         mask: [controlPage.getByTestId('timer-countdown')],
-        maxDiffPixelRatio: 0.2, // Relaxed to handle CI rendering differences
       })
 
       // Stop the timer to reset for the next test

tests/unit/useVolumePreference.test.ts

@@ -11,6 +11,9 @@ jest.mock('@/utils/audioManager', () => ({
     setVolume: jest.fn(),
     setMuted: jest.fn(),
   },
+  clampVolume: jest.fn((val: number) =>
+    Math.min(100, Math.max(0, Math.round(val)))
+  ),
 }))
 
 describe('hooks/useVolumePreference', () => {

utils/audioManager.ts

@@ -5,7 +5,10 @@
  * @public
  */
 
-export class AudioManager {
+export const clampVolume = (value: number): number =>
+  Math.min(100, Math.max(0, Math.round(value)))
+
+class AudioManager {
   private shortBeep: HTMLAudioElement | null = null
   private longBeep: HTMLAudioElement | null = null
   private isMuted = false