Skip to content

Commit 2a4912f

Browse files
Implement Volume Control Consistency (#1541)
feat: Implement Volume Control Consistency This commit introduces a consistent volume control experience across the application. Key changes: - Created a new presentational `VolumeSlider` component with mute functionality. - Refactored `SpotifyDisplay.tsx` to use the new `VolumeSlider` and synchronize its state with real-time `spotifyData` from the WebSocket. - Refactored `app/client/spotify-selection/page.tsx` to use the new `VolumeSlider` and the `useVolumePreference` hook. - Refactored `app/client/control/components/SpotifyControls.tsx` to use the new `VolumeSlider`. - Implemented debouncing for all `SET_VOLUME` commands to prevent API flooding. - Removed the old `VolumeControl.tsx` component and its associated test file. - Fixed all related unit tests and build errors. I was unable to complete the frontend verification step due to issues with the Playwright script and the test environment's authentication. The script was unable to trigger the necessary state to render the volume controls. The core functionality is implemented and unit tested. feat: Address PR feedback for volume control consistency This commit addresses the feedback from the pull request review for the volume control consistency feature. Key changes: - Fixed the missing debounce on the `/client/control` page by implementing a debounced `sendVolumeCommand` in `SpotifyControls.tsx`. - Improved the accessibility and responsiveness of the `VolumeSlider.tsx` component by replacing `aria-labelledby` with a direct `aria-label` and using a flexible width. - Added the missing export for the new `VolumeSlider.tsx` component to `components/index.ts`. - Added a new unit test file for `VolumeSlider.tsx` to verify its rendering, props, and interactions. - Resolved all build errors and ensured all unit tests are passing. Frontend verification was attempted but not completed due to issues with the test environment's WebSocket connection. The core functionality has been implemented and unit tested. fix: Address PR feedback and fix linting errors This commit addresses the feedback from the pull request review for the volume control consistency feature, and also fixes all linting errors. Key changes: - Fixed the missing debounce on the `/client/control` page by implementing a debounced `sendVolumeCommand` in `SpotifyControls.tsx`. - Improved the accessibility and responsiveness of the `VolumeSlider.tsx` component by replacing `aria-labelledby` with a direct `aria-label` and using a flexible width. - Added the missing export for the new `VolumeSlider.tsx` component to `components/index.ts`. - Added a new unit test file for `VolumeSlider.tsx` to verify its rendering, props, and interactions. - Resolved all build errors and ensured all unit tests are passing. - Fixed all linting errors by running `pnpm run lint:fix`. fix: Address final PR feedback and remove extraneous file This commit addresses the final feedback from the pull request review for the volume control consistency feature. Key changes: - Removed the extraneous `hrm@0.10.0` file. - All previous feedback has been addressed, including debouncing, accessibility, responsiveness, unit tests, and linting. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
1 parent 492f26d commit 2a4912f

12 files changed

Lines changed: 250 additions & 199 deletions

File tree

app/client/control/components/SpotifyControls.tsx

Lines changed: 28 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
11
// File: app/client/control/components/SpotifyControls.tsx
22
'use client'
33
import MusicNote from '@mui/icons-material/MusicNote'
4-
import VolumeOff from '@mui/icons-material/VolumeOff'
5-
import VolumeUp from '@mui/icons-material/VolumeUp'
64
import LibraryMusic from '@mui/icons-material/LibraryMusic'
75
import Box from '@mui/material/Box'
86
import Button from '@mui/material/Button'
@@ -11,16 +9,14 @@ import CardContent from '@mui/material/CardContent'
119
import FormControl from '@mui/material/FormControl'
1210
import MenuItem from '@mui/material/MenuItem'
1311
import Select from '@mui/material/Select'
14-
import Slider from '@mui/material/Slider'
15-
import Stack from '@mui/material/Stack'
1612
import Typography from '@mui/material/Typography'
17-
import IconButton from '@mui/material/IconButton'
1813
import { useRouter } from 'next/navigation'
1914
import { useCallback, useEffect, useRef, useState } from 'react'
2015
import useVolumePreference, { clampVolume } from '@/hooks/useVolumePreference'
2116
import { useWebSocket } from '@/context/WebSocketContext'
2217
import { SpotifyCommand, SpotifyCommandMessage } from '@/types/websocket'
2318
import PlaybackControls from './PlaybackControls'
19+
import VolumeSlider from '@/components/Spotify/VolumeSlider'
2420

2521
const SpotifyControls = () => {
2622
const router = useRouter()
@@ -30,7 +26,6 @@ const SpotifyControls = () => {
3026
const { volume, setVolume, muted, toggleMute } = useVolumePreference()
3127
const lastSentVolumeRef = useRef<string | null>(null)
3228
const [selectedDeviceId, setSelectedDeviceId] = useState<string>('')
33-
const [isDragging, setIsDragging] = useState(false)
3429
const prevActiveIdRef = useRef<string | undefined>(undefined)
3530

3631
const handleBrowseClick = () => {
@@ -77,11 +72,7 @@ const SpotifyControls = () => {
7772
prevActiveIdRef.current = activeId
7873

7974
// Sync Volume (if not dragging)
80-
if (
81-
!isDragging &&
82-
activeDevice &&
83-
typeof activeDevice.volume_percent === 'number'
84-
) {
75+
if (activeDevice && typeof activeDevice.volume_percent === 'number') {
8576
if (activeDevice.volume_percent !== volume) {
8677
setVolume(activeDevice.volume_percent)
8778
}
@@ -154,14 +145,32 @@ const SpotifyControls = () => {
154145
[connectionStatus, resolveTargetDeviceId, sendData]
155146
)
156147

148+
const debounceTimeoutRef = useRef<NodeJS.Timeout | null>(null)
149+
157150
useEffect(() => {
158151
if (connectionStatus !== 'Connected') {
159152
lastSentVolumeRef.current = null
160153
}
161154
}, [connectionStatus])
162155

163156
useEffect(() => {
164-
sendVolumeCommand(volume)
157+
// Clear any existing timer
158+
if (debounceTimeoutRef.current) {
159+
clearTimeout(debounceTimeoutRef.current)
160+
}
161+
162+
// Set a new timer to send the volume command after 300ms
163+
debounceTimeoutRef.current = setTimeout(() => {
164+
sendVolumeCommand(volume)
165+
}, 300)
166+
167+
// Cleanup function to clear the timeout if the component unmounts
168+
// or if the volume changes again before the timeout has passed
169+
return () => {
170+
if (debounceTimeoutRef.current) {
171+
clearTimeout(debounceTimeoutRef.current)
172+
}
173+
}
165174
}, [volume, sendVolumeCommand])
166175

167176
return (
@@ -208,41 +217,13 @@ const SpotifyControls = () => {
208217
disabled={connectionStatus !== 'Connected'}
209218
/>
210219

211-
<Stack direction="row" spacing={1} alignItems="center">
212-
<IconButton
213-
onClick={toggleMute}
214-
aria-label={muted ? 'Unmute volume' : 'Mute volume'}
215-
size="small"
216-
sx={{ color: 'grey.400' }}
217-
>
218-
{muted ? <VolumeOff /> : <VolumeUp />}
219-
</IconButton>
220-
<Slider
221-
aria-label="Volume control"
222-
value={volume}
223-
onChange={(_, val) => {
224-
setIsDragging(true)
225-
setVolume(val as number)
226-
}}
227-
onChangeCommitted={(_, val) => {
228-
setIsDragging(false)
229-
sendVolumeCommand(val as number)
230-
}}
231-
min={0}
232-
max={100}
233-
size="small"
234-
sx={{
235-
color: '#1DB954',
236-
'& .MuiSlider-thumb': { backgroundColor: 'white' },
237-
}}
238-
/>
239-
<Typography
240-
variant="caption"
241-
sx={{ color: 'grey.400', minWidth: '3ch' }}
242-
>
243-
{volume}
244-
</Typography>
245-
</Stack>
220+
<VolumeSlider
221+
volume={volume}
222+
muted={muted}
223+
onVolumeChange={setVolume}
224+
onToggleMute={toggleMute}
225+
/>
226+
246227
{devices.length > 0 && (
247228
<Box sx={{ mt: 2 }}>
248229
<Typography variant="body2" sx={{ color: 'grey.400', mb: 1 }}>

app/client/spotify-selection/page.tsx

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,8 @@ import Skeleton from '@mui/material/Skeleton'
1414
import Stack from '@mui/material/Stack'
1515
import Typography from '@mui/material/Typography'
1616
import dynamic from 'next/dynamic'
17-
import { useEffect, useState } from 'react'
18-
import VolumeControl from '../../../components/Spotify/VolumeControl' // I will recreate this temporarily
17+
import { useEffect, useState, useRef } from 'react'
18+
import VolumeSlider from '../../../components/Spotify/VolumeSlider'
1919
import useVolumePreference from '../../../hooks/useVolumePreference'
2020
import { useWebSocket } from '@/context/WebSocketContext'
2121
import { SpotifyCommandMessage } from '../../../types/websocket'
@@ -74,7 +74,8 @@ const SpotifySelectionPage = () => {
7474
setSelectedDeviceId('')
7575
}
7676
}, [availableDevices, selectedDeviceId])
77-
const { volume, setVolume } = useVolumePreference()
77+
const { volume, setVolume, muted, toggleMute } = useVolumePreference()
78+
const debounceTimeoutRef = useRef<NodeJS.Timeout | null>(null)
7879

7980
const handlePlaylistSelected = (uri: string) => {
8081
setSelectedPlaylistUri(uri)
@@ -197,13 +198,21 @@ const SpotifySelectionPage = () => {
197198
Next
198199
</Button>
199200
</Stack>
200-
<VolumeControl
201+
<VolumeSlider
201202
volume={volume}
202-
onVolumeChange={setVolume}
203-
onVolumeChangeCommitted={(newVolume) =>
204-
hasActiveDevice &&
205-
sendSpotifyCommand('SET_VOLUME', { volume: newVolume })
206-
}
203+
muted={muted}
204+
onVolumeChange={(newVolume) => {
205+
setVolume(newVolume)
206+
if (debounceTimeoutRef.current) {
207+
clearTimeout(debounceTimeoutRef.current)
208+
}
209+
debounceTimeoutRef.current = setTimeout(() => {
210+
if (hasActiveDevice) {
211+
sendSpotifyCommand('SET_VOLUME', { volume: newVolume })
212+
}
213+
}, 300)
214+
}}
215+
onToggleMute={toggleMute}
207216
/>
208217
{/* Device dropdown */}
209218
{availableDevices.length > 0 && (

components/Spotify/VolumeControl.tsx

Lines changed: 0 additions & 47 deletions
This file was deleted.
Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
// components/Spotify/VolumeSlider.tsx
2+
import React from 'react'
3+
import { IconButton, Slider, Stack, Typography } from '@mui/material'
4+
import { VolumeUp, VolumeOff } from '@mui/icons-material'
5+
6+
interface VolumeSliderProps {
7+
volume: number
8+
muted: boolean
9+
onVolumeChange: (volume: number) => void
10+
onToggleMute: () => void
11+
showValue?: boolean
12+
}
13+
14+
const VolumeSlider: React.FC<VolumeSliderProps> = ({
15+
volume,
16+
muted,
17+
onVolumeChange,
18+
onToggleMute,
19+
showValue = true,
20+
}) => {
21+
return (
22+
<Stack
23+
direction="row"
24+
spacing={1}
25+
alignItems="center"
26+
sx={{ flexGrow: 1, minWidth: 150 }}
27+
>
28+
<IconButton
29+
onClick={onToggleMute}
30+
size="small"
31+
aria-label={muted ? 'Unmute volume' : 'Mute volume'}
32+
>
33+
{muted || volume === 0 ? <VolumeOff /> : <VolumeUp />}
34+
</IconButton>
35+
<Slider
36+
value={muted ? 0 : volume}
37+
onChange={(_, val) => onVolumeChange(val as number)}
38+
min={0}
39+
max={100}
40+
size="small"
41+
aria-label="Volume control"
42+
/>
43+
{showValue && (
44+
<Typography
45+
variant="caption"
46+
sx={{ minWidth: '3ch', textAlign: 'right' }}
47+
>
48+
{muted ? '0' : volume}
49+
</Typography>
50+
)}
51+
</Stack>
52+
)
53+
}
54+
55+
export default VolumeSlider

0 commit comments

Comments
 (0)