Skip to content

Commit e12b0b4

Browse files
fix: resolve arpeggio widget playback leak on close or stop (#7739)
* fix: resolve arpeggio widget playback leak on close or stop * test: cover __playNote early return when not playing
1 parent eb99bea commit e12b0b4

2 files changed

Lines changed: 118 additions & 1 deletion

File tree

js/widgets/__tests__/arpeggio.test.js

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -537,6 +537,14 @@ describe("Arpeggio Widget", () => {
537537

538538
expect(arpeggio._playing).toBe(false);
539539
});
540+
541+
test("returns immediately if not playing", () => {
542+
arpeggio._playing = false;
543+
const triggerSpy = jest.spyOn(activityMock.logo.synth, "trigger");
544+
arpeggio.__playNote(0);
545+
expect(triggerSpy).not.toHaveBeenCalled();
546+
triggerSpy.mockRestore();
547+
});
540548
});
541549

542550
// --- __playCell fallback Tests ---
@@ -616,4 +624,73 @@ describe("Arpeggio Widget", () => {
616624
expect(cell2.style.backgroundColor).toBe("black");
617625
});
618626
});
627+
628+
// --- Playback Teardown Tests ---
629+
describe("playback teardown and stop behavior", () => {
630+
beforeEach(() => {
631+
jest.useFakeTimers();
632+
arpeggio.notesToPlay = [["C4", 1]];
633+
arpeggio.init(activityMock);
634+
});
635+
636+
afterEach(() => {
637+
jest.useRealTimers();
638+
});
639+
640+
test("widgetWindow.onclose stops playback and clears timeout", () => {
641+
// Start playing
642+
const cell = document.getElementById("2,1");
643+
cell.onclick({ target: cell });
644+
arpeggio.playButton.onclick();
645+
expect(arpeggio._playing).toBe(true);
646+
expect(arpeggio._playTimeout).not.toBeNull();
647+
648+
const timeoutSpy = jest.spyOn(global, "clearTimeout");
649+
650+
// Close window
651+
arpeggio.widgetWindow.onclose();
652+
653+
expect(arpeggio._playing).toBe(false);
654+
expect(arpeggio._playTimeout).toBeNull();
655+
expect(timeoutSpy).toHaveBeenCalled();
656+
expect(activityMock.logo.synth.stop).toHaveBeenCalled();
657+
expect(mockWidgetWindow.destroy).toHaveBeenCalled();
658+
659+
timeoutSpy.mockRestore();
660+
});
661+
662+
test("clicking play button when playing stops playback and clears timeout", () => {
663+
const cell = document.getElementById("2,1");
664+
cell.onclick({ target: cell });
665+
arpeggio.playButton.onclick();
666+
expect(arpeggio._playing).toBe(true);
667+
expect(arpeggio._playTimeout).not.toBeNull();
668+
669+
const timeoutSpy = jest.spyOn(global, "clearTimeout");
670+
671+
// Click to stop
672+
arpeggio.playButton.onclick();
673+
674+
expect(arpeggio._playing).toBe(false);
675+
expect(arpeggio._playTimeout).toBeNull();
676+
expect(timeoutSpy).toHaveBeenCalled();
677+
expect(activityMock.logo.synth.stop).toHaveBeenCalled();
678+
679+
timeoutSpy.mockRestore();
680+
});
681+
682+
test("_clear stops playback, clears timeout, and resets play button state", () => {
683+
const cell = document.getElementById("2,1");
684+
cell.onclick({ target: cell });
685+
arpeggio.playButton.onclick();
686+
expect(arpeggio._playing).toBe(true);
687+
688+
// Clear widget
689+
arpeggio._clear();
690+
691+
expect(arpeggio._playing).toBe(false);
692+
expect(arpeggio._playTimeout).toBeNull();
693+
expect(activityMock.logo.synth.stop).toHaveBeenCalled();
694+
});
695+
});
619696
});

js/widgets/arpeggio.js

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ class Arpeggio {
4545
// These arrays get created each time the matrix is built.
4646
this._blockMap = []; // pairs storage
4747
this.defaultCols = Arpeggio.DEFAULTCOLS;
48+
this._playTimeout = null;
4849
}
4950

5051
/**
@@ -119,6 +120,13 @@ class Arpeggio {
119120

120121
// For the button callbacks
121122
widgetWindow.onclose = () => {
123+
if (this._playTimeout) {
124+
clearTimeout(this._playTimeout);
125+
this._playTimeout = null;
126+
}
127+
this._playing = false;
128+
this._activity.logo.synth.stop();
129+
122130
arpeggioTableDiv.style.visibility = "hidden";
123131
this._activity.hideMsgs();
124132
widgetWindow.destroy();
@@ -522,6 +530,11 @@ class Arpeggio {
522530
})(),
523531
document.createTextNode("\u00a0\u00a0")
524532
);
533+
if (this._playTimeout) {
534+
clearTimeout(this._playTimeout);
535+
this._playTimeout = null;
536+
}
537+
this._activity.logo.synth.stop();
525538
this._playing = false;
526539
return;
527540
}
@@ -593,6 +606,9 @@ class Arpeggio {
593606
* @returns {void}
594607
*/
595608
__playNote(i) {
609+
if (!this._playing) {
610+
return;
611+
}
596612
if (i < this._playList.length) {
597613
if (this._playList[i][0].length > 0) {
598614
this._activity.logo.synth.trigger(
@@ -605,7 +621,7 @@ class Arpeggio {
605621
null
606622
);
607623
}
608-
setTimeout(() => {
624+
this._playTimeout = setTimeout(() => {
609625
this.__playNote(i + 1);
610626
}, 2600 * this._playList[i][1]);
611627
} else {
@@ -734,6 +750,30 @@ class Arpeggio {
734750
* @returns {void}
735751
*/
736752
_clear() {
753+
if (this._playing) {
754+
this._playing = false;
755+
if (this._playTimeout) {
756+
clearTimeout(this._playTimeout);
757+
this._playTimeout = null;
758+
}
759+
this._activity.logo.synth.stop();
760+
const icon = this.playButton;
761+
icon.replaceChildren(
762+
document.createTextNode("\u00a0\u00a0"),
763+
(() => {
764+
const img = document.createElement("img");
765+
img.src = "header-icons/play-button.svg";
766+
img.title = _("Play");
767+
img.alt = _("Play");
768+
img.height = Arpeggio.ICONSIZE;
769+
img.width = Arpeggio.ICONSIZE;
770+
img.style.verticalAlign = "middle";
771+
img.style.alignContent = "center";
772+
return img;
773+
})(),
774+
document.createTextNode("\u00a0\u00a0")
775+
);
776+
}
737777
// "Unclick" every entry in the matrix.
738778
const arpeggioTable = docById("arpeggioTable");
739779
let table;

0 commit comments

Comments
 (0)