Skip to content

Commit

Permalink
refactor: after review
Browse files Browse the repository at this point in the history
  • Loading branch information
PKulkoRaccoonGang committed Nov 27, 2024
1 parent 310421d commit 4c2d5bf
Show file tree
Hide file tree
Showing 14 changed files with 365 additions and 207 deletions.
2 changes: 1 addition & 1 deletion src/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,5 +78,5 @@ export const REGEX_RULES = {
};

export const IFRAME_FEATURE_POLICY = (
'microphone *; camera *; midi *; geolocation *; encrypted-media *, clipboard-write *'
'microphone *; camera *; midi *; geolocation *; encrypted-media *; clipboard-write *'
);
13 changes: 5 additions & 8 deletions src/course-unit/CourseUnit.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -1687,15 +1687,11 @@ describe('<CourseUnit />', () => {

waitFor(() => {
const iframe = getByTitle(xblockContainerIframeMessages.xblockIframeTitle.defaultMessage);
const currentXBlockId = courseVerticalChildrenMock.children[0].block_id;
const usageId = courseVerticalChildrenMock.children[0].block_id;
expect(iframe).toBeInTheDocument();

simulatePostMessageEvent(messageTypes.currentXBlockId, {
id: currentXBlockId,
});

simulatePostMessageEvent(messageTypes.manageXBlockAccess, {
id: currentXBlockId,
usageId,
});
});

Expand All @@ -1717,7 +1713,7 @@ describe('<CourseUnit />', () => {
const iframe = getByTitle(xblockContainerIframeMessages.xblockIframeTitle.defaultMessage);
expect(iframe).toBeInTheDocument();
simulatePostMessageEvent(messageTypes.manageXBlockAccess, {
id: courseVerticalChildrenMock.children[0].block_id,
usageId: courseVerticalChildrenMock.children[0].block_id,
});
});

Expand Down Expand Up @@ -1752,7 +1748,7 @@ describe('<CourseUnit />', () => {
const iframe = getByTitle(xblockContainerIframeMessages.xblockIframeTitle.defaultMessage);
expect(iframe).toBeInTheDocument();
simulatePostMessageEvent(messageTypes.manageXBlockAccess, {
id: courseVerticalChildrenMock.children[0].block_id,
usageId: courseVerticalChildrenMock.children[0].block_id,
});
});

Expand Down Expand Up @@ -1816,6 +1812,7 @@ describe('<CourseUnit />', () => {

waitFor(() => {
simulatePostMessageEvent(messageTypes.duplicateXBlock, {});
simulatePostMessageEvent(messageTypes.newXBlockEditor, {});
expect(mockedUsedNavigate)
.toHaveBeenCalledWith(`/course/${courseId}/editor/html/${targetBlockId}`, { replace: true });
});
Expand Down
7 changes: 6 additions & 1 deletion src/course-unit/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ export const messageTypes = {
duplicateXBlock: 'duplicateXBlock',
refreshXBlockPositions: 'refreshPositions',
newXBlockEditor: 'newXBlockEditor',
currentXBlockId: 'currentXBlockId',
toggleCourseXBlockDropdown: 'toggleCourseXBlockDropdown',
};

Expand All @@ -75,3 +74,9 @@ export const COMPONENT_TYPES = {
video: 'video',
dragAndDrop: 'drag-and-drop-v2',
};

export const COMPONENT_TYPES_WITH_NEW_EDITOR = {
html: 'html',
problem: 'problem',
video: 'video',
};
5 changes: 5 additions & 0 deletions src/course-unit/xblock-container-iframe/hooks/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export { useIframeMessages } from './useIframeMessages';
export { useIframeContent } from './useIframeContent';
export { useMessageHandlers } from './useMessageHandlers';
export { useIFrameBehavior } from './useIFrameBehavior';
export { useLoadBearingHook } from './useLoadBearingHook';
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ import { renderHook, act } from '@testing-library/react-hooks';
import { useKeyedState } from '@edx/react-unit-test-utils';
import { logError } from '@edx/frontend-platform/logging';

import { stateKeys, messageTypes } from '../../constants';
import { useIFrameBehavior, useLoadBearingHook } from '../hooks';
import { stateKeys, messageTypes } from '../../../constants';
import { useLoadBearingHook, useIFrameBehavior } from '..';

jest.mock('@edx/react-unit-test-utils', () => ({
useKeyedState: jest.fn(),
Expand Down
25 changes: 25 additions & 0 deletions src/course-unit/xblock-container-iframe/hooks/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
export type UseMessageHandlersTypes = {
courseId: string;
navigate: (path: string) => void;
dispatch: (action: any) => void;
setIframeOffset: (height: number) => void;
handleDeleteXBlock: (usageId: string) => void;
handleRefetchXBlocks: () => void;
handleDuplicateXBlock: (blockType: string, usageId: string) => void;
handleManageXBlockAccess: (usageId: string) => void;
};

export type MessageHandlersTypes = Record<string, (payload: any) => void>;

export interface UseIFrameBehaviorTypes {
id: string;
iframeUrl: string;
onLoaded?: boolean;
}

export interface UseIFrameBehaviorReturnTypes {
iframeHeight: number;
handleIFrameLoad: () => void;
showError: boolean;
hasLoaded: boolean;
}
Original file line number Diff line number Diff line change
@@ -1,57 +1,12 @@
import {
useState, useLayoutEffect, useCallback, useEffect,
} from 'react';
import { useCallback, useEffect } from 'react';
import { logError } from '@edx/frontend-platform/logging';
// eslint-disable-next-line import/no-extraneous-dependencies
import { useKeyedState } from '@edx/react-unit-test-utils';

import { useEventListener } from '../../generic/hooks';
import { stateKeys, messageTypes } from '../constants';

interface UseIFrameBehaviorParams {
id: string;
iframeUrl: string;
onLoaded?: boolean;
}

interface UseIFrameBehaviorReturn {
iframeHeight: number;
handleIFrameLoad: () => void;
showError: boolean;
hasLoaded: boolean;
}

/**
* We discovered an error in Firefox where - upon iframe load - React would cease to call any
* useEffect hooks until the user interacts with the page again. This is particularly confusing
* when navigating between sequences, as the UI partially updates leaving the user in a nebulous
* state.
*
* We were able to solve this error by using a layout effect to update some component state, which
* executes synchronously on render. Somehow this forces React to continue it's lifecycle
* immediately, rather than waiting for user interaction. This layout effect could be anywhere in
* the parent tree, as far as we can tell - we chose to add a conspicuously 'load bearing' (that's
* a joke) one here so it wouldn't be accidentally removed elsewhere.
*
* If we remove this hook when one of these happens:
* 1. React figures out that there's an issue here and fixes a bug.
* 2. We cease to use an iframe for unit rendering.
* 3. Firefox figures out that there's an issue in their iframe loading and fixes a bug.
* 4. We stop supporting Firefox.
* 5. An enterprising engineer decides to create a repo that reproduces the problem, submits it to
* Firefox/React for review, and they kindly help us figure out what in the world is happening
* so we can fix it.
*
* This hook depends on the unit id just to make sure it re-evaluates whenever the ID changes. If
* we change whether or not the Unit component is re-mounted when the unit ID changes, this may
* become important, as this hook will otherwise only evaluate the useLayoutEffect once.
*/
export const useLoadBearingHook = (id: string): void => {
const setValue = useState(0)[1];
useLayoutEffect(() => {
setValue(currentValue => currentValue + 1);
}, [id]);
};
import { useEventListener } from '../../../generic/hooks';
import { stateKeys, messageTypes } from '../../constants';
import { useLoadBearingHook } from './useLoadBearingHook';
import { UseIFrameBehaviorTypes, UseIFrameBehaviorReturnTypes } from './types';

/**
* Custom hook to manage iframe behavior.
Expand All @@ -70,7 +25,7 @@ export const useIFrameBehavior = ({
id,
iframeUrl,
onLoaded = true,
}: UseIFrameBehaviorParams): UseIFrameBehaviorReturn => {
}: UseIFrameBehaviorTypes): UseIFrameBehaviorReturnTypes => {
// Do not remove this hook. See function description.
useLoadBearingHook(id);

Expand Down
33 changes: 33 additions & 0 deletions src/course-unit/xblock-container-iframe/hooks/useIframeContent.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import { useEffect, useCallback, RefObject } from 'react';

import { messageTypes } from '../../constants';

/**
* Hook for managing iframe content and providing utilities to interact with the iframe.
*
* @param {React.RefObject<HTMLIFrameElement>} iframeRef - A React ref for the iframe element.
* @param {(ref: React.RefObject<HTMLIFrameElement>) => void} setIframeRef -
* A function to associate the iframeRef with the parent context.
* @param {(type: string, payload: any) => void} sendMessageToIframe - A function to send messages to the iframe.
*
* @returns {Object} - An object containing utility functions.
* @returns {() => void} return.refreshIframeContent -
* A function to refresh the iframe content by sending a specific message.
*/
export const useIframeContent = (
iframeRef: RefObject<HTMLIFrameElement>,
setIframeRef: (ref: RefObject<HTMLIFrameElement>) => void,
sendMessageToIframe: (type: string, payload: any) => void,
): { refreshIframeContent: () => void } => {
useEffect(() => {
setIframeRef(iframeRef);
}, [setIframeRef, iframeRef]);

// TODO: this artificial delay is a temporary solution
// to ensure the iframe content is properly refreshed.
const refreshIframeContent = useCallback(() => {
setTimeout(() => sendMessageToIframe(messageTypes.refreshXBlock, null), 1000);
}, [sendMessageToIframe]);

return { refreshIframeContent };
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { useEffect } from 'react';

/**
* Hook for managing and handling messages received by the iframe.
*
* @param {Record<string, (payload: any) => void>} messageHandlers -
* A mapping of message types to their corresponding handler functions.
*/
export const useIframeMessages = (messageHandlers: Record<string, (payload: any) => void>) => {
useEffect(() => {
const handleMessage = (event: MessageEvent) => {
const { type, payload } = event.data || {};
console.log('event.data ===============>', event.data);

Check warning on line 13 in src/course-unit/xblock-container-iframe/hooks/useIframeMessages.tsx

View workflow job for this annotation

GitHub Actions / tests

Unexpected console statement
if (type in messageHandlers) {
messageHandlers[type](payload);
}
};
window.addEventListener('message', handleMessage);
return () => window.removeEventListener('message', handleMessage);
}, [messageHandlers]);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import { useLayoutEffect, useState } from 'react';

/**
* We discovered an error in Firefox where - upon iframe load - React would cease to call any
* useEffect hooks until the user interacts with the page again. This is particularly confusing
* when navigating between sequences, as the UI partially updates leaving the user in a nebulous
* state.
*
* We were able to solve this error by using a layout effect to update some component state, which
* executes synchronously on render. Somehow this forces React to continue it's lifecycle
* immediately, rather than waiting for user interaction. This layout effect could be anywhere in
* the parent tree, as far as we can tell - we chose to add a conspicuously 'load bearing' (that's
* a joke) one here so it wouldn't be accidentally removed elsewhere.
*
* If we remove this hook when one of these happens:
* 1. React figures out that there's an issue here and fixes a bug.
* 2. We cease to use an iframe for unit rendering.
* 3. Firefox figures out that there's an issue in their iframe loading and fixes a bug.
* 4. We stop supporting Firefox.
* 5. An enterprising engineer decides to create a repo that reproduces the problem, submits it to
* Firefox/React for review, and they kindly help us figure out what in the world is happening
* so we can fix it.
*
* This hook depends on the unit id just to make sure it re-evaluates whenever the ID changes. If
* we change whether or not the Unit component is re-mounted when the unit ID changes, this may
* become important, as this hook will otherwise only evaluate the useLayoutEffect once.
*/
export const useLoadBearingHook = (id: string): void => {
const setValue = useState(0)[1];
useLayoutEffect(() => {
setValue(currentValue => currentValue + 1);
}, [id]);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import { useMemo } from 'react';

import { copyToClipboard } from '../../../generic/data/thunks';
import { messageTypes } from '../../constants';
import { MessageHandlersTypes, UseMessageHandlersTypes } from './types';

/**
* Hook for creating message handlers used to handle iframe messages.
*
* @param params - The parameters required to create message handlers.
* @returns {MessageHandlersTypes} - An object mapping message types to their handler functions.
*/
export const useMessageHandlers = ({
courseId,
navigate,
dispatch,
setIframeOffset,
handleDeleteXBlock,
handleRefetchXBlocks,
handleDuplicateXBlock,
handleManageXBlockAccess,
}: UseMessageHandlersTypes): MessageHandlersTypes => useMemo(() => ({
[messageTypes.copyXBlock]: ({ usageId }) => dispatch(copyToClipboard(usageId)),
[messageTypes.deleteXBlock]: ({ usageId }) => handleDeleteXBlock(usageId),
[messageTypes.newXBlockEditor]: ({ blockType, usageId }) => navigate(`/course/${courseId}/editor/${blockType}/${usageId}`),
[messageTypes.duplicateXBlock]: ({ blockType, usageId }) => handleDuplicateXBlock(blockType, usageId),
[messageTypes.manageXBlockAccess]: ({ usageId }) => handleManageXBlockAccess(usageId),
[messageTypes.refreshXBlockPositions]: handleRefetchXBlocks,
[messageTypes.toggleCourseXBlockDropdown]: ({
courseXBlockDropdownHeight,
}: { courseXBlockDropdownHeight: number }) => setIframeOffset(courseXBlockDropdownHeight),
}), [
courseId,
handleDeleteXBlock,
handleRefetchXBlocks,
handleDuplicateXBlock,
handleManageXBlockAccess,
]);
Loading

0 comments on commit 4c2d5bf

Please sign in to comment.