From b71b792e23d1857e8b578b3fbb3d3fb6913f967b Mon Sep 17 00:00:00 2001 From: "macroscopeapp[bot]" <170038800+macroscopeapp[bot]@users.noreply.github.com> Date: Sat, 24 Oct 2026 15:52:27 +0000 Subject: [PATCH] Fix DPI editor race: picking a preset cancels the pending debounced slider commit --- src/features/devices/cards/DpiCard.tsx | 23 +++-- .../devices/cards/dpi-edit-debounce.test.ts | 91 +++++++++++++++++++ .../devices/cards/dpi-edit-debounce.ts | 49 ++++++++++ 3 files changed, 151 insertions(+), 12 deletions(-) create mode 100644 src/features/devices/cards/dpi-edit-debounce.test.ts create mode 100644 src/features/devices/cards/dpi-edit-debounce.ts diff --git a/src/features/devices/cards/DpiCard.tsx b/src/features/devices/cards/DpiCard.tsx index d8e1a1b1..921026dd 100644 --- a/src/features/devices/cards/DpiCard.tsx +++ b/src/features/devices/cards/DpiCard.tsx @@ -9,6 +9,7 @@ import * as control from "../../../device/controller"; import type { ControlSnapshot, LiftOffLevel } from "../../../device/types"; import { isNoirE1Status, isNoirKsnakeStatus, isNoirS1Status, NOIR_DPI_STAGE_COLORS } from "../../../device/noir.ts"; import { closestDpiOption, dpiPresetValues } from "../../../device/dpi-presets"; +import { createDpiEditDebouncer } from "./dpi-edit-debounce"; import { t, tp } from "../../../i18n/index"; import { LiftOffDistance, hasLiftOff } from "./PerformanceCards"; import { IconLinked, IconUnlinked } from "../../../shared/icons"; @@ -110,13 +111,13 @@ function DpiStageEditor({ const rowsRef = useRef(rows); rowsRef.current = rows; const touchedRef = useRef(0); - const timerRef = useRef | null>(null); + const [edits] = useState(() => createDpiEditDebouncer()); useEffect(() => { return () => { - if (timerRef.current) clearTimeout(timerRef.current); + edits.cancel(); }; - }, []); + }, [edits]); const fixedStageCount = mode === "stage" && stageEditor?.countEditable !== true; const fillsStageTable = singleFillsStageTable(snapshot); @@ -160,7 +161,7 @@ function DpiStageEditor({ useEffect(() => { if (sourceSig === lastSigRef.current) return; lastSigRef.current = sourceSig; - if (Date.now() - touchedRef.current > 1500 || (mode === "logitech" && !timerRef.current)) setRows(initRows(rowsRef.current)); + if (Date.now() - touchedRef.current > 1500 || (mode === "logitech" && !edits.pending())) setRows(initRows(rowsRef.current)); // initRows is recreated each render on purpose so it always reads fresh snapshot data. // eslint-disable-next-line react-hooks/exhaustive-deps }, [sourceSig]); @@ -242,17 +243,11 @@ function DpiStageEditor({ const markEdited = (delay = 450, nextRows?: StageRow[]): void => { touchedRef.current = Date.now(); const snapshotRows = nextRows ?? rowsRef.current; - if (timerRef.current) clearTimeout(timerRef.current); - timerRef.current = setTimeout(() => { - timerRef.current = null; - commit(snapshotRows); - }, delay); + edits.schedule(() => commit(snapshotRows), delay); }; const flushEdit = (): void => { - if (!timerRef.current) return; - clearTimeout(timerRef.current); - timerRef.current = null; + if (!edits.cancel()) return; commit(rowsRef.current); }; @@ -262,6 +257,10 @@ function DpiStageEditor({ if (!enabled || next[index].enabled === rows[index].enabled) return; const snap = closestDpiOption(options, parseRow(next[index].value) ?? status?.dpi ?? 0); if (snap === null) return; + // A slider or typed edit may still have a debounced commit pending with + // the rows it captured. The pick is newer, so drop that commit or it + // would re-apply the old row's DPI over this one when it fires. + edits.cancel(); const selected = next.map((row, i) => (i === index ? { ...row, enabled: true } : { ...row, enabled: false })); setRows(selected); rowsRef.current = selected; diff --git a/src/features/devices/cards/dpi-edit-debounce.test.ts b/src/features/devices/cards/dpi-edit-debounce.test.ts new file mode 100644 index 00000000..6292a823 --- /dev/null +++ b/src/features/devices/cards/dpi-edit-debounce.test.ts @@ -0,0 +1,91 @@ +import assert from "node:assert/strict"; +import { test } from "node:test"; + +import { createDpiEditDebouncer, type EditTimers } from "./dpi-edit-debounce.ts"; + +function fakeTimers(): EditTimers & { advance(ms: number): void; active(): number } { + let now = 0; + let nextId = 1; + const queue = new Map void }>(); + return { + setTimeout(callback, delay) { + const id = nextId++; + queue.set(id, { at: now + delay, callback }); + return id; + }, + clearTimeout(handle) { + queue.delete(handle as number); + }, + advance(ms) { + now += ms; + for (const [id, entry] of [...queue].sort((a, b) => a[1].at - b[1].at)) { + if (entry.at > now || !queue.has(id)) continue; + queue.delete(id); + entry.callback(); + } + }, + active: () => queue.size, + }; +} + +test("a scheduled commit runs once after its delay", () => { + const timers = fakeTimers(); + const edits = createDpiEditDebouncer(timers); + const runs: string[] = []; + edits.schedule(() => runs.push("a"), 450); + assert.equal(edits.pending(), true); + timers.advance(449); + assert.deepEqual(runs, []); + timers.advance(1); + assert.deepEqual(runs, ["a"]); + assert.equal(edits.pending(), false); + timers.advance(1000); + assert.deepEqual(runs, ["a"]); +}); + +test("a newer schedule replaces the pending commit", () => { + const timers = fakeTimers(); + const edits = createDpiEditDebouncer(timers); + const runs: string[] = []; + edits.schedule(() => runs.push("old"), 450); + timers.advance(200); + edits.schedule(() => runs.push("new"), 450); + assert.equal(timers.active(), 1); + timers.advance(1000); + assert.deepEqual(runs, ["new"]); +}); + +test("cancel drops the pending commit and reports whether one was pending", () => { + const timers = fakeTimers(); + const edits = createDpiEditDebouncer(timers); + const runs: string[] = []; + assert.equal(edits.cancel(), false); + edits.schedule(() => runs.push("a"), 450); + assert.equal(edits.cancel(), true); + assert.equal(edits.pending(), false); + assert.equal(edits.cancel(), false); + timers.advance(1000); + assert.deepEqual(runs, []); +}); + +test("a preset picked after a slider edit is not overwritten by the slider's stale rows", () => { + const timers = fakeTimers(); + const edits = createDpiEditDebouncer(timers); + const applied: number[] = []; + + // Slider on the active row moves to 1600; the commit captures those rows. + const snapshotRows = [{ enabled: true, value: "1600" }, { enabled: false, value: "3200" }]; + edits.schedule(() => { + const active = snapshotRows.find((row) => row.enabled); + if (active) applied.push(Number(active.value)); + }, 450); + + // The user picks the 3200 preset before the debounce fires, as the single + // DPI view does: drop the pending commit, then apply the pick right away. + timers.advance(100); + edits.cancel(); + applied.push(3200); + + timers.advance(1000); + assert.deepEqual(applied, [3200]); +}); diff --git a/src/features/devices/cards/dpi-edit-debounce.ts b/src/features/devices/cards/dpi-edit-debounce.ts new file mode 100644 index 00000000..45a7d05e --- /dev/null +++ b/src/features/devices/cards/dpi-edit-debounce.ts @@ -0,0 +1,49 @@ +export interface EditTimers { + setTimeout(callback: () => void, delay: number): unknown; + clearTimeout(handle: unknown): void; +} + +const globalTimers: EditTimers = { + setTimeout: (callback, delay) => setTimeout(callback, delay), + clearTimeout: (handle) => clearTimeout(handle as ReturnType), +}; + +/** + * Holds at most one pending DPI editor commit. A newer schedule replaces the + * pending one, and anything applied immediately (a preset pick) must cancel it + * first so the older rows captured by the timer cannot land on top of it. + */ +export interface DpiEditDebouncer { + /** Run `run` after `delay` ms, dropping any commit still pending. */ + schedule(run: () => void, delay: number): void; + /** Drop the pending commit without running it. Returns whether one was pending. */ + cancel(): boolean; + pending(): boolean; +} + +export function createDpiEditDebouncer(timers: EditTimers = globalTimers): DpiEditDebouncer { + let handle: unknown = null; + let scheduled = false; + + const cancel = (): boolean => { + if (!scheduled) return false; + timers.clearTimeout(handle); + handle = null; + scheduled = false; + return true; + }; + + return { + schedule(run, delay) { + cancel(); + scheduled = true; + handle = timers.setTimeout(() => { + handle = null; + scheduled = false; + run(); + }, delay); + }, + cancel, + pending: () => scheduled, + }; +}