diff --git a/app/client/control/components/SpotifyControls.tsx b/app/client/control/components/SpotifyControls.tsx index de132092a8..da32c162da 100644 --- a/app/client/control/components/SpotifyControls.tsx +++ b/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,14 +80,12 @@ 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, @@ -93,11 +93,6 @@ const spotifyControlsReducer = ( } } } - 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('') const lastSentVolumeRef = useRef(null) const lastWarningTimeRef = useRef(0) const prevActiveIdRef = useRef(undefined) - const lastVolumeSyncTimeRef = useRef(0) - const hasPendingSendRef = useRef(false) + const pendingSendTimeoutRef = useRef | 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,7 +192,8 @@ 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 } @@ -228,13 +201,14 @@ const SpotifyControls = () => { // 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,7 +321,6 @@ const SpotifyControls = () => { ) const handleToggleMute = useCallback(() => { - // Calculate the next state to determine the command payload const newMutedState = !isMuted const newVolume = newMutedState ? 0 @@ -350,7 +328,7 @@ const SpotifyControls = () => { ? 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 ( { 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) } diff --git a/components/SpotifyDisplay.tsx b/components/SpotifyDisplay.tsx index dab1a61b69..fd728c1a66 100644 --- a/components/SpotifyDisplay.tsx +++ b/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' diff --git a/hooks/useVolumePreference.ts b/hooks/useVolumePreference.ts index 3b7a8809fd..be2fcd4c81 100644 --- a/hooks/useVolumePreference.ts +++ b/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) diff --git a/tests/playwright/lib/masks.ts b/tests/playwright/lib/masks.ts index 172aeb3c33..c9d872677f 100644 --- a/tests/playwright/lib/masks.ts +++ b/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 diff --git a/tests/playwright/vrt-components.spec.ts b/tests/playwright/vrt-components.spec.ts index 0c042483e2..5ad70b5930 100644 --- a/tests/playwright/vrt-components.spec.ts +++ b/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 diff --git a/tests/playwright/vrt-dashboard.spec.ts b/tests/playwright/vrt-dashboard.spec.ts index 80d868aa71..74095b1c40 100644 --- a/tests/playwright/vrt-dashboard.spec.ts +++ b/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, }) }) }) diff --git a/tests/playwright/vrt-timer-controls.spec.ts b/tests/playwright/vrt-timer-controls.spec.ts index e43a648b32..031e2743f7 100644 --- a/tests/playwright/vrt-timer-controls.spec.ts +++ b/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 diff --git a/tests/unit/useVolumePreference.test.ts b/tests/unit/useVolumePreference.test.ts index 71fbbe06f8..eb4274a561 100644 --- a/tests/unit/useVolumePreference.test.ts +++ b/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', () => { diff --git a/utils/audioManager.ts b/utils/audioManager.ts index 11a9ea4f8c..09021f9e2c 100644 --- a/utils/audioManager.ts +++ b/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