diff --git a/web/src/App.vue b/web/src/App.vue index 8a1e0cb..ea8c0e9 100644 --- a/web/src/App.vue +++ b/web/src/App.vue @@ -44,11 +44,15 @@ type="range" min="0" max="100" - :value="mobileVolume" + :value="mobileVolumeDisplay" class="m-volume-slider" - @input="onMobileVolumeChange" + @input="onMobileVolumeInput" + @change="onMobileVolumeCommit" + @pointerup="onMobileVolumeRelease" + @pointercancel="onMobileVolumeRelease" + @blur="onMobileVolumeRelease" /> - {{ mobileVolume }} + {{ mobileVolumeDisplay }} @@ -79,6 +83,7 @@ import { computed, onMounted, onUnmounted, ref } from 'vue'; import { useRoute, useRouter } from 'vue-router'; import { Icon } from '@iconify/vue'; import { usePlayerStore } from './stores/player.js'; +import { useDecoupledSlider } from './composables/useDecoupledSlider.js'; import { useWebSocket } from './composables/useWebSocket.js'; import { useSession } from './composables/useSession.js'; import Navbar from './components/Navbar.vue'; @@ -99,7 +104,17 @@ const route = useRoute(); const router = useRouter(); const { connect } = useWebSocket(); const currentSong = computed(() => playerStore.currentSong); -const mobileVolume = computed(() => playerStore.activeBot?.volume ?? 75); +// Volume slider decoupled from the 60fps updateMobileProgress() rAF re-render +// so it isn't reset mid-drag (#111 — same root cause as the desktop player). +const { + display: mobileVolumeDisplay, + onInput: onMobileVolumeInput, + onChange: onMobileVolumeCommit, + onRelease: onMobileVolumeRelease, +} = useDecoupledSlider( + () => playerStore.activeBot?.volume, + (v) => playerStore.setVolume(v) +); const mobileMode = computed(() => playerStore.activeBot?.playMode ?? 'seq'); const mobileModeOrder = ['seq', 'loop', 'random', 'rloop']; const mobileModeIcons: Record = { @@ -126,11 +141,6 @@ function updateMobileProgress() { mobileRaf = requestAnimationFrame(updateMobileProgress); } -function onMobileVolumeChange(e: Event) { - const volume = Number((e.target as HTMLInputElement).value); - playerStore.setVolume(volume); -} - function toggleMobileVolume() { mobileVolumeOpen.value = !mobileVolumeOpen.value; if (mobileVolumeOpen.value) mobileQueueOpen.value = false; diff --git a/web/src/components/Player.vue b/web/src/components/Player.vue index 138013c..fbec185 100644 --- a/web/src/components/Player.vue +++ b/web/src/components/Player.vue @@ -65,8 +65,12 @@ type="range" min="0" max="100" - :value="activeBot?.volume ?? 75" + :value="volumeDisplay" + @input="onVolumeInput" @change="onVolumeChange" + @pointerup="onVolumeRelease" + @pointercancel="onVolumeRelease" + @blur="onVolumeRelease" class="volume-slider" /> @@ -87,6 +91,7 @@ import { Icon } from '@iconify/vue'; import { useRoute, useRouter } from 'vue-router'; import { usePlayerStore } from '../stores/player.js'; import { useSession } from '../composables/useSession.js'; +import { useDecoupledSlider } from '../composables/useDecoupledSlider.js'; import CoverArt from './CoverArt.vue'; import Queue from './Queue.vue'; @@ -185,10 +190,17 @@ function togglePlay() { } } -function onVolumeChange(e: Event) { - const target = e.target as HTMLInputElement; - store.setVolume(parseInt(target.value)); -} +// Volume slider is decoupled from the per-frame rAF re-render so dragging the +// thumb isn't reset every frame (#111). See useDecoupledSlider. +const { + display: volumeDisplay, + onInput: onVolumeInput, + onChange: onVolumeChange, + onRelease: onVolumeRelease, +} = useDecoupledSlider( + () => activeBot.value?.volume, + (v) => store.setVolume(v) +); const modeOrder = ['seq', 'loop', 'random', 'rloop'] as const; const modeIcons: Record = { diff --git a/web/src/composables/useDecoupledSlider.test.ts b/web/src/composables/useDecoupledSlider.test.ts new file mode 100644 index 0000000..5e4b8f1 --- /dev/null +++ b/web/src/composables/useDecoupledSlider.test.ts @@ -0,0 +1,84 @@ +import { describe, it, expect, vi } from "vitest"; +import { ref, nextTick } from "vue"; +import { useDecoupledSlider } from "./useDecoupledSlider.js"; + +/** Minimal stand-in for an change/input Event. */ +function ev(value: number): Event { + return { target: { value: String(value) } } as unknown as Event; +} + +describe("useDecoupledSlider (#111)", () => { + it("initialises display from the source, falling back when undefined", () => { + const src = ref(40); + const { display } = useDecoupledSlider(() => src.value, () => {}); + expect(display.value).toBe(40); + + const empty = useDecoupledSlider(() => undefined, () => {}, 75); + expect(empty.display.value).toBe(75); + }); + + it("reflects external source changes into the display when not dragging", async () => { + const src = ref(50); + const { display } = useDecoupledSlider(() => src.value, () => {}); + src.value = 80; + await nextTick(); + expect(display.value).toBe(80); + }); + + it("tracks @input locally without committing", () => { + const commit = vi.fn(); + const { display, onInput } = useDecoupledSlider(() => 50, commit); + onInput(ev(63)); + expect(display.value).toBe(63); + expect(commit).not.toHaveBeenCalled(); + }); + + // THE REGRESSION: this is exactly what the 60fps rAF re-render did — push the + // (stale) source value back into the binding mid-drag. The guard must ignore + // it so the thumb stays where the user dragged it. + it("ignores external source changes WHILE dragging (no snap-back)", async () => { + const src = ref(50); + const { display, onInput } = useDecoupledSlider(() => src.value, () => {}); + + onInput(ev(70)); // user starts dragging → display 70 + expect(display.value).toBe(70); + + // Simulate the per-frame re-render re-evaluating the (still-stale) source. + src.value = 50; + await nextTick(); + expect(display.value).toBe(70); // stayed put — did NOT snap back to 50 + }); + + // Corner case: a range input skips `change` when released back at its start + // value. onRelease (pointerup/pointercancel/blur) must still end the drag so + // the slider doesn't freeze against later external updates. + it("clears dragging on release even when @change never fires", async () => { + const src = ref(50); + const { display, onInput, onRelease } = useDecoupledSlider(() => src.value, () => {}); + + onInput(ev(70)); // drag begins + onInput(ev(50)); // ...dragged back to the start value + onRelease(); // released — browser emits NO change event here + + src.value = 30; // a later external update + await nextTick(); + expect(display.value).toBe(30); // slider resumed following the source + }); + + it("commits on @change and resumes following the source afterwards", async () => { + const commit = vi.fn(); + const src = ref(50); + const { display, onInput, onChange } = useDecoupledSlider(() => src.value, commit); + + onInput(ev(70)); + onChange(ev(70)); // release + expect(commit).toHaveBeenCalledTimes(1); + expect(commit).toHaveBeenCalledWith(70); + expect(display.value).toBe(70); + + // After release, external changes flow through again. + src.value = 35; + await nextTick(); + expect(display.value).toBe(35); + }); +}); diff --git a/web/src/composables/useDecoupledSlider.ts b/web/src/composables/useDecoupledSlider.ts new file mode 100644 index 0000000..9329571 --- /dev/null +++ b/web/src/composables/useDecoupledSlider.ts @@ -0,0 +1,67 @@ +import { ref, watch, type Ref } from 'vue'; + +/** + * A slider whose displayed value is decoupled from its reactive source. + * + * Why this exists (#111): the player components run a 60fps requestAnimationFrame + * loop (progress clock, #107) that re-renders the whole component every ~16ms. + * Binding a range `` straight to a reactive source (the store + * volume) let Vue re-apply `el.value = source` on every one of those re-renders. + * Mid-drag the source is still the *old* value, so the native drag position kept + * getting snapped back — the thumb was effectively un-draggable on desktop and + * janky on mobile. + * + * The fix is to bind `:value` to a LOCAL ref that: + * - tracks the native drag synchronously via `@input` (so the bound value always + * equals the element's value → Vue never resets it), and + * - is committed to the real source only on `@change` (release). + * External source changes (bot switch, another client) still flow into the + * display — except while the user is actively dragging, where they must be + * ignored or they'd fight the drag. + * + * @param source getter for the authoritative value (e.g. () => bot?.volume) + * @param commit called with the final value on release (e.g. store.setVolume) + * @param fallback value to show when the source is undefined (default 75) + */ +export function useDecoupledSlider( + source: () => number | undefined, + commit: (value: number) => void, + fallback = 75 +): { + display: Ref; + dragging: Ref; + onInput: (e: Event) => void; + onChange: (e: Event) => void; + onRelease: () => void; +} { + const display = ref(source() ?? fallback); + const dragging = ref(false); + + watch(source, (v) => { + // Reflect external/store changes — but never while dragging, or the + // per-frame re-render would yank the thumb away from the user's finger. + if (!dragging.value && typeof v === 'number') display.value = v; + }); + + function onInput(e: Event): void { + dragging.value = true; + display.value = Number((e.target as HTMLInputElement).value); + } + + function onChange(e: Event): void { + dragging.value = false; + const v = Number((e.target as HTMLInputElement).value); + display.value = v; + commit(v); + } + + function onRelease(): void { + // Safety net for pointerup / pointercancel / blur: a range input does NOT + // emit `change` if the value is released back at its starting point, which + // would otherwise leave `dragging` stuck true and freeze the slider against + // later external updates. Clearing here is idempotent with onChange. + dragging.value = false; + } + + return { display, dragging, onInput, onChange, onRelease }; +}