diff --git a/client/app/bundles/course/assessment/submission/components/ScribingView/ScribingCanvas.tsx b/client/app/bundles/course/assessment/submission/components/ScribingView/ScribingCanvas.tsx index b89f67065f0..38cbe1cb03b 100644 --- a/client/app/bundles/course/assessment/submission/components/ScribingView/ScribingCanvas.tsx +++ b/client/app/bundles/course/assessment/submission/components/ScribingView/ScribingCanvas.tsx @@ -1331,13 +1331,23 @@ const ScribingCanvas = forwardRef( className="flex justify-center-safe bg-neutral-300 m-0 w-full outline-none items-center" style={{ minWidth: 800 }} > - {!scribingState[answerId]?.isCanvasLoaded ? : null} + {/* + * Rendered AFTER the canvas, never before: once Fabric.js constructs the + * canvas, it wraps the raw node in its own container div, + * re-parenting it out from under this div's direct children. A sibling + * rendered BEFORE the canvas would need React to `insertBefore` relative + * to that no-longer-direct-child node on every toggle (e.g. a redundant + * re-initialize resetting isCanvasLoaded) — which throws. Appending a + * trailing sibling never needs a reference node, so it's safe regardless + * of what Fabric has done to the canvas by then. + */} + {!scribingState[answerId]?.isCanvasLoaded ? : null} ); }, diff --git a/client/app/bundles/course/assessment/submission/components/ScribingView/__test__/index.test.tsx b/client/app/bundles/course/assessment/submission/components/ScribingView/__test__/index.test.tsx index e09fc1d8272..0d0dc8506f1 100644 --- a/client/app/bundles/course/assessment/submission/components/ScribingView/__test__/index.test.tsx +++ b/client/app/bundles/course/assessment/submission/components/ScribingView/__test__/index.test.tsx @@ -1,12 +1,37 @@ import { dispatch } from 'store'; -import { act, render } from 'test-utils'; +import { act, render, waitFor } from 'test-utils'; import { QuestionType } from 'types/course/assessment/question'; import { ScribingAnswerData } from 'types/course/assessment/submission/answer/scribing'; import ScribingView from 'course/assessment/submission/containers/ScribingView'; +import { LOADING_INDICATOR_TEST_ID } from 'lib/components/core/LoadingIndicator'; import { scribingActions } from '../../../reducers/scribing'; +/** + * jsdom never actually fetches/decodes images, so a real 's `load` event + * never fires for a fake asset URL — ScribingCanvas's own image-load effect + * (where it actually constructs the Fabric.js canvas) never runs under the + * default Image. This stub returns a REAL HTMLImageElement (so Fabric/canvas's + * `instanceof` checks on drawImage's source still pass), with `src` patched to + * fire `load` asynchronously, like a real browser would. + */ +function StubImage(): HTMLImageElement { + const img = document.createElement('img'); + Object.defineProperty(img, 'width', { value: 100, configurable: true }); + Object.defineProperty(img, 'height', { value: 100, configurable: true }); + let currentSrc = ''; + Object.defineProperty(img, 'src', { + configurable: true, + get: () => currentSrc, + set: (value: string) => { + currentSrc = value; + setTimeout(() => img.dispatchEvent(new Event('load')), 0); + }, + }); + return img; +} + const assessmentId = 1; const submissionId = 2; const answerId = 3; @@ -74,4 +99,76 @@ describe('ScribingView', () => { await page.findByTestId(`canvas-${answerId}`, {}, { timeout: 5000 }), ).toBeVisible(); }); + + describe('with a real Fabric.js canvas', () => { + const OriginalImage = global.Image; + + beforeAll(() => { + // @ts-expect-error — minimal stub, see StubImage above + global.Image = StubImage; + }); + + afterAll(() => { + global.Image = OriginalImage; + }); + + it('survives a redundant re-initialize after the canvas has already loaded', async () => { + // React reports this crash as an uncaught commit-phase error (via a + // dispatched DOM event), not as a rejected promise — `dispatch`/`act` + // above it don't throw. Catch it directly so the test fails clearly + // instead of just timing out waiting for a DOM update that never comes. + const uncaughtErrors: string[] = []; + const onWindowError = (event: ErrorEvent): void => { + uncaughtErrors.push(event.error?.message ?? event.message); + }; + window.addEventListener('error', onWindowError); + + try { + await act(() => + dispatch( + scribingActions.initialize({ answers: mockSubmission.answers }), + ), + ); + + const url = `/courses/${global.courseId}/assessments/${assessmentId}/submissions/${submissionId}/edit`; + const page = render(, { + at: [url], + }); + + // Wait for the component's own image-load effect to construct the real + // Fabric.js canvas — this is what wraps the in Fabric's own + // container div, re-parenting it out from under React's tree. + await waitFor( + () => + expect( + page.queryByTestId(LOADING_INDICATOR_TEST_ID), + ).not.toBeInTheDocument(), + { timeout: 5000 }, + ); + + // A duplicate FETCH_SUBMISSION_SUCCESS re-runs scribing/initialize, + // resetting isCanvasLoaded to false for an answer whose canvas Fabric.js + // has already taken over. Toggling the loading indicator back on then + // requires React to insertBefore a sibling relative to that + // no-longer-direct-child node — must not throw. + await act(() => + dispatch( + scribingActions.initialize({ answers: mockSubmission.answers }), + ), + ); + + expect(uncaughtErrors).toEqual([]); + expect( + await page.findByTestId( + LOADING_INDICATOR_TEST_ID, + {}, + { timeout: 5000 }, + ), + ).toBeVisible(); + expect(page.getByTestId(`canvas-${answerId}`)).toBeInTheDocument(); + } finally { + window.removeEventListener('error', onWindowError); + } + }); + }); }); diff --git a/client/app/bundles/course/assessment/submission/reducers/__test__/recorder.test.js b/client/app/bundles/course/assessment/submission/reducers/__test__/recorder.test.js new file mode 100644 index 00000000000..65e2f182937 --- /dev/null +++ b/client/app/bundles/course/assessment/submission/reducers/__test__/recorder.test.js @@ -0,0 +1,59 @@ +import recorderHelper from '../../../utils/recorderHelper'; +import actionTypes from '../../constants'; +import reducer from '../recorder'; + +jest.mock('../../../utils/recorderHelper'); + +describe('recorder reducer', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + describe('RECORDER_COMPONENT_UNMOUNT', () => { + it('does not try to stop the recorder when nothing was recording', () => { + recorderHelper.isRecording.mockReturnValue(false); + const state = { + recording: false, + recorderComponentsCount: 1, + recordingComponentId: '', + }; + + reducer(state, { type: actionTypes.RECORDER_COMPONENT_UNMOUNT }); + + expect(recorderHelper.stopRecord).not.toHaveBeenCalled(); + }); + + it('stops the recorder when the user navigates away mid-recording', () => { + recorderHelper.isRecording.mockReturnValue(true); + recorderHelper.stopRecord.mockResolvedValue(new File([], 'test.wav')); + const state = { + recording: true, + recorderComponentsCount: 1, + recordingComponentId: 'voice_response_1', + }; + + reducer(state, { type: actionTypes.RECORDER_COMPONENT_UNMOUNT }); + + expect(recorderHelper.stopRecord).toHaveBeenCalled(); + }); + + it('does not decrement below the last unmount and resets recording state', () => { + recorderHelper.isRecording.mockReturnValue(false); + const state = { + recording: true, + recorderComponentsCount: 1, + recordingComponentId: 'voice_response_1', + }; + + const nextState = reducer(state, { + type: actionTypes.RECORDER_COMPONENT_UNMOUNT, + }); + + expect(nextState).toEqual({ + recording: false, + recorderComponentsCount: 0, + recordingComponentId: 'voice_response_1', + }); + }); + }); +}); diff --git a/client/app/bundles/course/assessment/submission/reducers/recorder.js b/client/app/bundles/course/assessment/submission/reducers/recorder.js index 59f8d1d2f8c..9b1a89a94d0 100644 --- a/client/app/bundles/course/assessment/submission/reducers/recorder.js +++ b/client/app/bundles/course/assessment/submission/reducers/recorder.js @@ -38,10 +38,10 @@ export default function (state = initialState, action) { recording = false; /** - * When the user navigate to other path without stopping the recorder - * We need to help the user to stop + * When the user navigates to another path without stopping the recorder, + * help them stop it, but only if it was actually recording. */ - if (recorderComponentsCount === 0) { + if (recorderComponentsCount === 0 && recorderHelper.isRecording()) { recorderHelper.stopRecord(); } return {