From 5207934abfa0ec44d2e59c750480fa5def78c015 Mon Sep 17 00:00:00 2001 From: "D. Ror" Date: Thu, 6 Jun 2024 16:38:53 -0400 Subject: [PATCH 1/6] Prevent cross-entry recording interference --- .../Pronunciations/AudioRecorder.tsx | 23 +++++++------- src/components/Pronunciations/Recorder.ts | 30 ++++++++++++++---- .../Pronunciations/RecorderIcon.tsx | 31 ++++++++++++++----- 3 files changed, 59 insertions(+), 25 deletions(-) diff --git a/src/components/Pronunciations/AudioRecorder.tsx b/src/components/Pronunciations/AudioRecorder.tsx index d6da37e9ee..7a9b0389c9 100644 --- a/src/components/Pronunciations/AudioRecorder.tsx +++ b/src/components/Pronunciations/AudioRecorder.tsx @@ -2,10 +2,8 @@ import { ReactElement, useContext } from "react"; import { useTranslation } from "react-i18next"; import { toast } from "react-toastify"; -import Recorder from "components/Pronunciations/Recorder"; import RecorderContext from "components/Pronunciations/RecorderContext"; import RecorderIcon from "components/Pronunciations/RecorderIcon"; -import { getFileNameForWord } from "components/Pronunciations/utilities"; import { StoreState } from "types"; import { useAppSelector } from "types/hooks"; import { FileWithSpeakerId } from "types/word"; @@ -26,31 +24,32 @@ export default function AudioRecorder(props: RecorderProps): ReactElement { const { t } = useTranslation(); async function startRecording(): Promise { + const isRecordingId = recorder.isRecording(); + if (isRecordingId && isRecordingId !== props.id) { + // Prevent interfering with an active recording on a different entry. + return; + } + // Prevent starting a recording before a previous one is finished. await stopRecording(); - recorder.startRecording(); + recorder.startRecording(props.id); } - async function stopRecording(): Promise { + async function stopRecording(): Promise { // Prevent triggering this function if no recording is active. - if (!recorder.isRecording()) { + if (recorder.isRecording() === undefined) { return; } if (props.onClick) { props.onClick(); } - const blob = await recorder.stopRecording(); - if (!blob) { + const file = await recorder.stopRecording(); + if (!file) { toast.error(t("pronunciations.noMicAccess")); return; } - const fileName = getFileNameForWord(props.id); - const file = new File([blob], fileName, { - lastModified: Date.now(), - type: Recorder.blobType, - }); if (!props.noSpeaker) { (file as FileWithSpeakerId).speakerId = speakerId; } diff --git a/src/components/Pronunciations/Recorder.ts b/src/components/Pronunciations/Recorder.ts index b044d118d3..a1432d6174 100644 --- a/src/components/Pronunciations/Recorder.ts +++ b/src/components/Pronunciations/Recorder.ts @@ -1,8 +1,11 @@ import RecordRTC from "recordrtc"; +import { getFileNameForWord } from "components/Pronunciations/utilities"; + export default class Recorder { private toast: (text: string) => void; private recordRTC?: RecordRTC; + private id?: string; static blobType: RecordRTC.Options["type"] = "audio"; @@ -14,21 +17,36 @@ export default class Recorder { .catch((err) => this.onError(err)); } - isRecording(): boolean { - return this.recordRTC?.getState() === "recording"; + /** Checks if the recorder state is `"recording"`. + * If so, returns the `id` used with `startRecording()`. + * If not, returns `undefined`. */ + isRecording(): string | undefined { + return this.recordRTC?.getState() === "recording" + ? this.id ?? "" + : undefined; } - startRecording(): void { + startRecording(id: string): void { this.recordRTC?.reset(); + this.id = id; this.recordRTC?.startRecording(); } - stopRecording(): Promise { - return new Promise((resolve) => { + stopRecording(): Promise { + return new Promise((resolve) => { const rec = this.recordRTC; if (rec) { - rec.stopRecording(() => resolve(rec.getBlob())); + rec.stopRecording(() => { + const fileName = getFileNameForWord(this.id ?? ""); + const file = new File([rec.getBlob()], fileName, { + lastModified: Date.now(), + type: Recorder.blobType, + }); + this.id = undefined; + resolve(file); + }); } else { + this.id = undefined; resolve(undefined); } }); diff --git a/src/components/Pronunciations/RecorderIcon.tsx b/src/components/Pronunciations/RecorderIcon.tsx index 8c45f676b1..19c1c3314a 100644 --- a/src/components/Pronunciations/RecorderIcon.tsx +++ b/src/components/Pronunciations/RecorderIcon.tsx @@ -25,20 +25,37 @@ interface RecorderIconProps { export default function RecorderIcon(props: RecorderIconProps): ReactElement { const isRecording = useAppSelector( (state: StoreState) => - state.pronunciationsState.status === PronunciationsStatus.Recording && - state.pronunciationsState.wordId === props.id + state.pronunciationsState.status === PronunciationsStatus.Recording ); + const recordingId = useAppSelector( + (state: StoreState) => state.pronunciationsState.wordId + ); + const isRecordingThis = isRecording && recordButtonId === props.id; const dispatch = useAppDispatch(); const { t } = useTranslation(); function toggleIsRecordingToTrue(): void { - dispatch(recording(props.id)); - props.startRecording(); + if (!isRecording) { + // Only start a recording if there's not another on in progress. + dispatch(recording(props.id)); + props.startRecording(); + } else { + // This triggers if user clicks-and-holds on one entry's record icon, + // drags the mouse outside that icon before releasing, + // then clicks-and-holds a different entry's record icon. + if (recordingId !== props.id) { + console.error( + "Tried to record for an entry before finishing a recording on another entry." + ); + } + } } function toggleIsRecordingToFalse(): void { - props.stopRecording(); - dispatch(resetPronunciations()); + if (isRecordingThis) { + props.stopRecording(); + dispatch(resetPronunciations()); + } } function handleTouchStart(): void { @@ -82,7 +99,7 @@ export default function RecorderIcon(props: RecorderIconProps): ReactElement { color: (t) => props.disabled ? t.palette.grey[400] - : isRecording + : isRecordingThis ? themeColors.recordActive : themeColors.recordIdle, }} From aa416f9ca2eb8a6f8395e762f2eabe0c0e20e5f6 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Wed, 12 Jun 2024 13:45:27 -0400 Subject: [PATCH 2/6] Remove unused import --- src/components/Pronunciations/AudioRecorder.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/src/components/Pronunciations/AudioRecorder.tsx b/src/components/Pronunciations/AudioRecorder.tsx index aa2251a217..a35d2d3fc3 100644 --- a/src/components/Pronunciations/AudioRecorder.tsx +++ b/src/components/Pronunciations/AudioRecorder.tsx @@ -4,7 +4,6 @@ import { toast } from "react-toastify"; import RecorderContext from "components/Pronunciations/RecorderContext"; import RecorderIcon from "components/Pronunciations/RecorderIcon"; -import { getFileNameForWord } from "components/Pronunciations/utilities"; import { useAppSelector } from "rootRedux/hooks"; import { type StoreState } from "rootRedux/types"; import { FileWithSpeakerId } from "types/word"; From 6d49cbb46d0cbdb1a72bbee4604c07b3c1be7de5 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Mon, 24 Jun 2024 12:09:23 -0400 Subject: [PATCH 3/6] Fix RecorderIcon bug and tests --- .../Pronunciations/RecorderIcon.tsx | 2 +- .../tests/AudioRecorder.test.tsx | 53 ++----------------- 2 files changed, 5 insertions(+), 50 deletions(-) diff --git a/src/components/Pronunciations/RecorderIcon.tsx b/src/components/Pronunciations/RecorderIcon.tsx index 8adedbc9b8..7ecd2422d3 100644 --- a/src/components/Pronunciations/RecorderIcon.tsx +++ b/src/components/Pronunciations/RecorderIcon.tsx @@ -30,7 +30,7 @@ export default function RecorderIcon(props: RecorderIconProps): ReactElement { const recordingId = useAppSelector( (state: StoreState) => state.pronunciationsState.wordId ); - const isRecordingThis = isRecording && recordButtonId === props.id; + const isRecordingThis = isRecording && recordingId === props.id; const dispatch = useAppDispatch(); const { t } = useTranslation(); diff --git a/src/components/Pronunciations/tests/AudioRecorder.test.tsx b/src/components/Pronunciations/tests/AudioRecorder.test.tsx index 0d6827d169..3f1e1b2cfb 100644 --- a/src/components/Pronunciations/tests/AudioRecorder.test.tsx +++ b/src/components/Pronunciations/tests/AudioRecorder.test.tsx @@ -5,10 +5,7 @@ import configureMockStore from "redux-mock-store"; import { defaultState } from "components/App/DefaultState"; import AudioRecorder from "components/Pronunciations/AudioRecorder"; -import RecorderIcon, { - recordButtonId, - recordIconId, -} from "components/Pronunciations/RecorderIcon"; +import { recordIconId } from "components/Pronunciations/RecorderIcon"; import { PronunciationsStatus } from "components/Pronunciations/Redux/PronunciationsReduxTypes"; import { type StoreState } from "rootRedux/types"; import theme, { themeColors } from "types/theme"; @@ -27,50 +24,8 @@ function mockRecordingState(wordId: string): Partial { }; } -beforeAll(() => { - act(() => { - testRenderer = create( - - - - - - - - ); - }); -}); - -describe("Pronunciations", () => { - test("pointerDown and pointerUp", () => { - const mockStartRecording = jest.fn(); - const mockStopRecording = jest.fn(); - act(() => { - testRenderer = create( - - - - - - - - ); - }); - - expect(mockStartRecording).not.toHaveBeenCalled(); - testRenderer.root.findByProps({ id: recordButtonId }).props.onPointerDown(); - expect(mockStartRecording).toHaveBeenCalled(); - - expect(mockStopRecording).not.toHaveBeenCalled(); - testRenderer.root.findByProps({ id: recordButtonId }).props.onPointerUp(); - expect(mockStopRecording).toHaveBeenCalled(); - }); - - test("default style is idle", () => { +describe("AudioRecorder", () => { + test("default icon style is idle", () => { act(() => { testRenderer = create( @@ -86,7 +41,7 @@ describe("Pronunciations", () => { expect(icon.props.sx.color({})).toEqual(themeColors.recordIdle); }); - test("style depends on pronunciations state", () => { + test("icon style depends on pronunciations state", () => { const wordId = "1"; const mockStore2 = configureMockStore()(mockRecordingState(wordId)); act(() => { From b7403b32406358d8b3cd5bfba13ff12189a97631 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Mon, 24 Jun 2024 12:11:58 -0400 Subject: [PATCH 4/6] Add extracted and updated RecorderIcon tests --- .../tests/RecorderIcon.test.tsx | 99 +++++++++++++++++++ 1 file changed, 99 insertions(+) create mode 100644 src/components/Pronunciations/tests/RecorderIcon.test.tsx diff --git a/src/components/Pronunciations/tests/RecorderIcon.test.tsx b/src/components/Pronunciations/tests/RecorderIcon.test.tsx new file mode 100644 index 0000000000..519e147f7d --- /dev/null +++ b/src/components/Pronunciations/tests/RecorderIcon.test.tsx @@ -0,0 +1,99 @@ +import { StyledEngineProvider, ThemeProvider } from "@mui/material/styles"; +import { Provider } from "react-redux"; +import { + ReactTestInstance, + ReactTestRenderer, + act, + create, +} from "react-test-renderer"; +import configureMockStore from "redux-mock-store"; + +import { defaultState } from "components/App/DefaultState"; +import RecorderIcon, { + recordButtonId, +} from "components/Pronunciations/RecorderIcon"; +import { PronunciationsStatus } from "components/Pronunciations/Redux/PronunciationsReduxTypes"; +import { type StoreState } from "rootRedux/types"; +import theme from "types/theme"; + +let testRenderer: ReactTestRenderer; +let testButton: ReactTestInstance; + +function mockRecordingState(wordId: string): Partial { + return { + ...defaultState, + pronunciationsState: { + fileName: "", + status: PronunciationsStatus.Recording, + wordId, + }, + }; +} + +const mockWordId = "1234567890"; + +const mockStartRecording = jest.fn(); +const mockStopRecording = jest.fn(); + +const renderRecorderIcon = async (wordId = ""): Promise => { + await act(async () => { + testRenderer = create( + + + + + + + + ); + }); + testButton = testRenderer.root.findByProps({ id: recordButtonId }); +}; + +beforeEach(() => { + jest.resetAllMocks(); +}); + +describe("RecorderIcon", () => { + test("pointerDown records if no recording active", async () => { + await renderRecorderIcon(); + expect(mockStartRecording).not.toHaveBeenCalled(); + await act(async () => { + testButton.props.onPointerDown(); + }); + expect(mockStartRecording).toHaveBeenCalled(); + }); + + test("pointerUp stops recording", async () => { + await renderRecorderIcon(mockWordId); + expect(mockStopRecording).not.toHaveBeenCalled(); + await act(async () => { + testButton.props.onPointerUp(); + }); + expect(mockStopRecording).toHaveBeenCalled(); + }); + + test("pointerUp does nothing if no recording active", async () => { + await renderRecorderIcon(); + await act(async () => { + testButton.props.onPointerUp(); + }); + expect(mockStopRecording).not.toHaveBeenCalled(); + }); + + test("pointerUp does nothing if different word id", async () => { + await renderRecorderIcon("different-id"); + await act(async () => { + testButton.props.onPointerUp(); + }); + expect(mockStopRecording).not.toHaveBeenCalled(); + }); +}); From 23abbeaaae4965f5c8aa5948a19956cd2d6b5409 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Mon, 24 Jun 2024 13:27:04 -0400 Subject: [PATCH 5/6] Fix return type --- src/components/Pronunciations/tests/RecorderIcon.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/Pronunciations/tests/RecorderIcon.test.tsx b/src/components/Pronunciations/tests/RecorderIcon.test.tsx index 519e147f7d..91d308c8ce 100644 --- a/src/components/Pronunciations/tests/RecorderIcon.test.tsx +++ b/src/components/Pronunciations/tests/RecorderIcon.test.tsx @@ -35,7 +35,7 @@ const mockWordId = "1234567890"; const mockStartRecording = jest.fn(); const mockStopRecording = jest.fn(); -const renderRecorderIcon = async (wordId = ""): Promise => { +const renderRecorderIcon = async (wordId = ""): Promise => { await act(async () => { testRenderer = create( From a385e9e7013b94389ef8501f41f0b6fb5068b9a4 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Mon, 8 Jul 2024 17:26:26 -0400 Subject: [PATCH 6/6] Fix function names --- src/components/Pronunciations/AudioRecorder.tsx | 6 +++--- src/components/Pronunciations/Recorder.ts | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/components/Pronunciations/AudioRecorder.tsx b/src/components/Pronunciations/AudioRecorder.tsx index a35d2d3fc3..fcb8ad58ce 100644 --- a/src/components/Pronunciations/AudioRecorder.tsx +++ b/src/components/Pronunciations/AudioRecorder.tsx @@ -24,8 +24,8 @@ export default function AudioRecorder(props: RecorderProps): ReactElement { const { t } = useTranslation(); async function startRecording(): Promise { - const isRecordingId = recorder.isRecording(); - if (isRecordingId && isRecordingId !== props.id) { + const recordingId = recorder.getRecordingId(); + if (recordingId && recordingId !== props.id) { // Prevent interfering with an active recording on a different entry. return; } @@ -38,7 +38,7 @@ export default function AudioRecorder(props: RecorderProps): ReactElement { async function stopRecording(): Promise { // Prevent triggering this function if no recording is active. - if (recorder.isRecording() === undefined) { + if (recorder.getRecordingId() === undefined) { return; } diff --git a/src/components/Pronunciations/Recorder.ts b/src/components/Pronunciations/Recorder.ts index a1432d6174..38fe5b50d6 100644 --- a/src/components/Pronunciations/Recorder.ts +++ b/src/components/Pronunciations/Recorder.ts @@ -20,7 +20,7 @@ export default class Recorder { /** Checks if the recorder state is `"recording"`. * If so, returns the `id` used with `startRecording()`. * If not, returns `undefined`. */ - isRecording(): string | undefined { + getRecordingId(): string | undefined { return this.recordRTC?.getState() === "recording" ? this.id ?? "" : undefined;