Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Suspense for Image loading to reduce the cognitive overload in the code and also utilize the react18 concurrent rendering #632

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Use Suspense for Image loading to reduce the cognitive overload in the code and also utilize the react18 concurrent rendering #632

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f366907
Use Suspense for Image loading to reduce the cognitive overload in th…
MTRNord Nov 13, 2024
c252a52
Move the image tests to the correct test to make it work with the new…
MTRNord Nov 18, 2024
37af562
Prevent flash of reload due to invalidation happening when we dont ha…
MTRNord Nov 18, 2024
3db8ff3
Remove debug log
MTRNord Nov 18, 2024
b0c2695
Cleanup download code, add logs and add comments
MTRNord Nov 27, 2024
a3adeb6
Move the image suspense parts to its own wrapper
MTRNord Nov 27, 2024
fa4e57c
Ensure we dont try to cache the blob url as it changes every time. In…
MTRNord Nov 27, 2024
6d79fb4
Fix tests
MTRNord Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/react-sdk/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,14 @@
"react-beautiful-dnd": "^13.1.1",
"react-draggable": "^4.4.6",
"react-dropzone": "^14.2.9",
"react-error-boundary": "^4.1.2",
"react-hotkeys-hook": "^4.5.0",
"react-joyride": "^2.9.2",
"react-redux": "^9.1.2",
"react-transition-group": "^4.4.5",
"react-use": "^17.5.1",
"rxjs": "^7.8.1",
"swr": "^2.2.5",
"tinycolor2": "^1.6.0",
"yjs": "^13.6.19"
},
Expand Down
122 changes: 121 additions & 1 deletion packages/react-sdk/src/components/Whiteboard/Element/ConnectedElement.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/

import { MockedWidgetApi, mockWidgetApi } from '@matrix-widget-toolkit/testing';
import { render, screen } from '@testing-library/react';
import { fireEvent, render, screen, waitFor } from '@testing-library/react';
import { ComponentType, PropsWithChildren } from 'react';
import {
MockInstance,
Expand All @@ -31,6 +31,7 @@ import {
mockImageElement,
mockWhiteboardManager,
} from '../../../lib/testUtils/documentTestUtils';
import { ImageMimeType } from '../../../state';
import { LayoutStateProvider } from '../../Layout';
import { SlidesProvider } from '../../Layout/SlidesProvider';
import { SvgCanvas } from '../SvgCanvas';
Expand Down Expand Up @@ -112,4 +113,123 @@ describe('<ConnectedElement />', () => {
screen.queryByTestId('element-element-0-image'),
).not.toBeInTheDocument();
});

it('should render an error when an image is not available', async () => {
vi.mocked(URL.createObjectURL).mockReturnValue('');

// @ts-ignore ignore readonly prop for tests
widgetApi.widgetParameters.baseUrl = 'https://example.com';

render(
<ConnectedElement
id="element-0"
activeElementIds={['element-0']}
readOnly={false}
overrides={{}}
/>,
{ wrapper: Wrapper },
);

expect(
screen.getByTestId('element-element-0-skeleton'),
).toBeInTheDocument();

const errorContainer = await screen.findByTestId(
'element-element-0-error-container',
);
expect(errorContainer).toBeInTheDocument();

const imageElement = screen.queryByTestId('element-element-0-image');
expect(imageElement).not.toBeInTheDocument();

vi.mocked(URL.createObjectURL).mockReset();
});

it.each([
['example.gif', 'image/gif'],
['example.jpeg', 'image/jpeg'],
['example.png', 'image/png'],
['example.svg', 'image/svg+xml'],
] as [string, ImageMimeType][])(
'should render %s without exploding',
async (fileName, mimeType) => {
widgetApi = mockWidgetApi();
consoleSpy = vi.spyOn(console, 'error');

vi.mocked(URL.createObjectURL).mockReturnValue('http://...');

const imageElementMock = mockImageElement();
imageElementMock.fileName = fileName;
imageElementMock.mimeType = mimeType;

const { whiteboardManager } = mockWhiteboardManager({
slides: [['slide-0', [['element-0', imageElementMock]]]],
});

Wrapper = ({ children }) => (
<LayoutStateProvider>
<WhiteboardTestingContextProvider
whiteboardManager={whiteboardManager}
widgetApi={widgetApi}
>
<SlidesProvider>
<SvgCanvas
viewportWidth={whiteboardWidth}
viewportHeight={whiteboardHeight}
>
{children}
</SvgCanvas>
</SlidesProvider>
</WhiteboardTestingContextProvider>
</LayoutStateProvider>
);

// @ts-ignore ignore readonly prop for tests
widgetApi.widgetParameters.baseUrl = 'https://example.com';

render(
<ConnectedElement
id="element-0"
activeElementIds={['element-0']}
readOnly={false}
overrides={{}}
/>,
{ wrapper: Wrapper },
);

expect(
screen.getByTestId('element-element-0-skeleton'),
).toBeInTheDocument();
const imageElement = await screen.findByTestId('element-element-0-image');
expect(imageElement).toBeInTheDocument();
},
);

it('should render a skeleton until an image is loaded', async () => {
// @ts-ignore ignore readonly prop for tests
widgetApi.widgetParameters.baseUrl = 'https://example.com';
render(
<ConnectedElement
id="element-0"
activeElementIds={['element-0']}
readOnly={false}
overrides={{}}
/>,
{ wrapper: Wrapper },
);

expect(
screen.getByTestId('element-element-0-skeleton'),
).toBeInTheDocument();

const imageElement = await screen.findByTestId('element-element-0-image');
expect(imageElement).toBeInTheDocument();
fireEvent.load(imageElement);

await waitFor(() => {
expect(
screen.queryByTestId('element-element-0-skeleton'),
).not.toBeInTheDocument();
});
});
});
25 changes: 10 additions & 15 deletions packages/react-sdk/src/components/Whiteboard/Element/ConnectedElement.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
* limitations under the License.
*/

import { useWidgetApi } from '@matrix-widget-toolkit/react';
import { Elements } from '../../../state/types';
import { useElementOverride } from '../../ElementOverridesProvider';
import EllipseDisplay from '../../elements/ellipse/Display';
Expand All @@ -24,6 +23,14 @@ import PolylineDisplay from '../../elements/polyline/Display';
import RectangleDisplay from '../../elements/rectangle/Display';
import TriangleDisplay from '../../elements/triangle/Display';

export type OtherProps = {
active: boolean;
readOnly: boolean;
elementId: string;
activeElementIds: string[];
overrides: Elements;
};

export const ConnectedElement = ({
id,
readOnly = false,
Expand All @@ -35,13 +42,12 @@ export const ConnectedElement = ({
activeElementIds?: string[];
overrides?: Elements;
}) => {
const widgetApi = useWidgetApi();
const element = useElementOverride(id);
const isActive =
!readOnly && id
? activeElementIds.length === 1 && activeElementIds[0] === id
: false;
const otherProps = {
const otherProps: OtherProps = {
// TODO: Align names
active: isActive,
readOnly,
Expand All @@ -66,18 +72,7 @@ export const ConnectedElement = ({
return <TriangleDisplay {...element} {...otherProps} />;
}
} else if (element.type === 'image') {
if (widgetApi.widgetParameters.baseUrl === undefined) {
console.error('Image cannot be rendered due to missing base URL');
return null;
}

return (
<ImageDisplay
baseUrl={widgetApi.widgetParameters.baseUrl}
{...element}
{...otherProps}
/>
);
return <ImageDisplay element={element} otherProps={otherProps} />;
}
}

Expand Down
131 changes: 17 additions & 114 deletions packages/react-sdk/src/components/elements/image/ImageDisplay.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/

import { MockedWidgetApi, mockWidgetApi } from '@matrix-widget-toolkit/testing';
import { fireEvent, render, screen, waitFor } from '@testing-library/react';
import { render, screen } from '@testing-library/react';
import { ComponentType, PropsWithChildren } from 'react';
import {
afterEach,
Expand All @@ -31,7 +31,6 @@ import {
mockWhiteboardManager,
WhiteboardTestingContextProvider,
} from '../../../lib/testUtils/documentTestUtils';
import { ImageMimeType } from '../../../state';
import { LayoutStateProvider } from '../../Layout';
import { SlidesProvider } from '../../Layout/SlidesProvider';
import { whiteboardHeight, whiteboardWidth } from '../../Whiteboard';
Expand Down Expand Up @@ -86,121 +85,25 @@ describe('<ImageDisplay />', () => {
fetch.resetMocks();
});

it.each([
['example.gif', 'image/gif'],
['example.jpeg', 'image/jpeg'],
['example.png', 'image/png'],
['example.svg', 'image/svg+xml'],
] as [string, ImageMimeType][])(
'should render %s without exploding',
async (fileName, mimeType) => {
render(
<ImageDisplay
elementId="element-0"
type="image"
fileName={fileName}
mimeType={mimeType}
baseUrl="https://example.com"
mxc="mxc://example.com/test1234"
width={200}
height={300}
position={{ x: 23, y: 42 }}
active={false}
readOnly={false}
/>,
{ wrapper: Wrapper },
);

expect(
screen.getByTestId('element-element-0-skeleton'),
).toBeInTheDocument();
const imageElement = await screen.findByTestId('element-element-0-image');
expect(imageElement).toBeInTheDocument();
},
);

it('should render a skeleton until an image is loaded', async () => {
render(
<ImageDisplay
elementId="element-0"
type="image"
fileName="example.jpeg"
mimeType="image/jpeg"
baseUrl="https://example.com"
mxc="mxc://example.com/test1234"
width={200}
height={300}
position={{ x: 23, y: 42 }}
active={false}
readOnly={false}
/>,
{ wrapper: Wrapper },
);

expect(
screen.getByTestId('element-element-0-skeleton'),
).toBeInTheDocument();

const imageElement = await screen.findByTestId('element-element-0-image');
expect(imageElement).toBeInTheDocument();
fireEvent.load(imageElement);

await waitFor(() => {
expect(
screen.queryByTestId('element-element-0-skeleton'),
).not.toBeInTheDocument();
});
});

it('should render an error when an image is not available', async () => {
vi.mocked(URL.createObjectURL).mockReturnValue('');

render(
<ImageDisplay
elementId="element-0"
type="image"
fileName="example.jpeg"
mimeType="image/jpeg"
baseUrl="https://example.com"
mxc="mxc://example.com/test1234"
width={200}
height={300}
position={{ x: 23, y: 42 }}
active={false}
readOnly={false}
/>,
{ wrapper: Wrapper },
);

expect(
screen.getByTestId('element-element-0-skeleton'),
).toBeInTheDocument();

const errorContainer = await screen.findByTestId(
'element-element-0-error-container',
);
expect(errorContainer).toBeInTheDocument();

const imageElement = screen.queryByTestId('element-element-0-image');
expect(imageElement).not.toBeInTheDocument();

vi.mocked(URL.createObjectURL).mockReset();
});

it('should not have a context menu in read-only mode', () => {
render(
<ImageDisplay
elementId="element-0"
type="image"
fileName="example.jpeg"
mimeType="image/jpeg"
baseUrl="https://example.com"
mxc="mxc://example.com/test1234"
width={200}
height={300}
position={{ x: 23, y: 42 }}
active={false}
readOnly={true}
otherProps={{
active: false,
readOnly: true,
elementId: 'element-0',
activeElementIds: [],
overrides: {},
}}
element={{
type: 'image',
fileName: 'example.jpeg',
mimeType: 'image/jpeg',
mxc: 'mxc://example.com/test1234',
width: 200,
height: 300,
position: { x: 23, y: 42 },
}}
/>,
{ wrapper: Wrapper },
);
Expand Down
Loading