Skip to content
This repository was archived by the owner on May 17, 2023. It is now read-only.

Commit 05fb2dc

Browse files
cristiandoucecristiandouce
authored
DXDP-1904 - Fixing Avatar component infinite loop when image not provided and gravatar fails to load (#1733)
* Adding stories that reproduce the infinite loop issue * Fixing the infinite loop issue when image prop is not defined and gravatar fails * Remove the src attribute from the DOM element by using native removeAttribute() * Add unit tests for Avatar atom component
1 parent f96aaee commit 05fb2dc

File tree

5 files changed

+254
-24
lines changed

5 files changed

+254
-24
lines changed

‎core/components/_helpers/story-example.tsx‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
1-
import * as React from 'react'
2-
import styled from '../styled'
3-
import { colors } from '../tokens'
1+
import * as React from "react";
2+
3+
import styled from "../styled";
4+
import { colors } from "../tokens";
45

56
const Title = styled.div`
67
position: absolute;

‎core/components/atoms/avatar/avatar.story.tsx‎

Lines changed: 87 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
1-
import * as React from 'react'
2-
import { storiesOf } from '@storybook/react'
3-
import { Example, Stack } from '../../_helpers/story-helpers'
4-
import { Avatar } from '../../'
1+
import * as React from "react";
2+
3+
import { storiesOf } from "@storybook/react";
4+
5+
import { Avatar } from "../../";
6+
import { Example, Stack } from "../../_helpers/story-helpers";
57

68
const EXAMPLE_IMAGE =
79
'data:image/svg+xml;base64,PD94bWwgdmVyc2lvbj0iMS4wIiBlbmNvZGluZz0iVVRGLTgiPz4KPHN2ZyB3aWR0aD0iNDhweCIgaGVpZ2h0PSI0OHB4IiB2aWV3Qm94PSIwIDAgNDggNDgiIHZlcnNpb249IjEuMSIgeG1sbnM9Imh0dHA6Ly93d3cudzMub3JnLzIwMDAvc3ZnIiB4bWxuczp4bGluaz0iaHR0cDovL3d3dy53My5vcmcvMTk5OS94bGluayI+CiAgICA8IS0tIEdlbmVyYXRvcjogU2tldGNoIDUwLjIgKDU1MDQ3KSAtIGh0dHA6Ly93d3cuYm9oZW1pYW5jb2RpbmcuY29tL3NrZXRjaCAtLT4KICAgIDx0aXRsZT5IZWxwZXJzIC8gQXZhdGFycyAvIFR5cGVzIC8gTW9iaWxlPC90aXRsZT4KICAgIDxkZXNjPkNyZWF0ZWQgd2l0aCBTa2V0Y2guPC9kZXNjPgogICAgPGRlZnM+PC9kZWZzPgogICAgPGcgaWQ9IkhlbHBlcnMtLy1BdmF0YXJzLS8tVHlwZXMtLy1Nb2JpbGUiIHN0cm9rZT0ibm9uZSIgc3Ryb2tlLXdpZHRoPSIxIiBmaWxsPSJub25lIiBmaWxsLXJ1bGU9ImV2ZW5vZGQiPgogICAgICAgIDxnIGlkPSJHcm91cC0yIj4KICAgICAgICAgICAgPHJlY3QgaWQ9IlJlY3RhbmdsZSIgeD0iMCIgeT0iMCIgd2lkdGg9IjQ4IiBoZWlnaHQ9IjQ4Ij48L3JlY3Q+CiAgICAgICAgPC9nPgogICAgICAgIDxnIGlkPSJzZXJ2aWNlIiB0cmFuc2Zvcm09InRyYW5zbGF0ZSgxMC4wMDAwMDAsIDExLjAwMDAwMCkiIGZpbGwtcnVsZT0ibm9uemVybyI+CiAgICAgICAgICAgIDxwb2x5Z29uIGlkPSJGaWxsLTUzIiBmaWxsPSIjNDRDN0Y0IiBwb2ludHM9IjYuODAzNzg5NTUgMjEuMzk4OTkzIDMuOTk0NjE0NDcgMTkuNzczOTkzIDEyLjczNjA5MjQgNC42MDQxNjY2NyAxNS41NDUyNjc1IDYuMjI5MTY2NjciPjwvcG9seWdvbj4KICAgICAgICAgICAgPHBvbHlnb24gaWQ9IkZpbGwtNTQiIGZpbGw9IiNFQzU0MjQiIHBvaW50cz0iMjEuNDc2MTg4NyAyMS4zOTg5OTMgMTIuNzM0NzEwOCA2LjIyOTE2NjY3IDE1LjU0Mzg4NTkgNC42MDQxNjY2NyAyNC4yODUzNjM3IDE5Ljc3Mzk5MyI+PC9wb2x5Z29uPgogICAgICAgICAgICA8cGF0aCBkPSJNMTkuNTUyNTg2Niw1LjQxNjY2NjY3IEMxOS41NTI1ODY2LDguNDA4MjAxNDIgMTcuMTMyMTUwNiwxMC44MzMzMzMzIDE0LjE0NjM0MTUsMTAuODMzMzMzMyBDMTEuMTYwNTYyNCwxMC44MzMzMzMzIDguNzQwMDk2MjksOC40MDgyMDE0MiA4Ljc0MDA5NjI5LDUuNDE2NjY2NjcgQzguNzQwMDk2MjksMi40MjUxMzE5NSAxMS4xNjA1NjI0LDAgMTQuMTQ2MzQxNSwwIEMxNy4xMzIxNTA2LDAgMTkuNTUyNTg2NiwyLjQyNTEzMTk1IDE5LjU1MjU4NjYsNS40MTY2NjY2NyIgaWQ9IkZpbGwtNTUiIGZpbGw9IiMxNjIxNEQiPjwvcGF0aD4KICAgICAgICAgICAgPHBhdGggZD0iTTEwLjgxMjQ5MDMsMjAuNTgzMzMzMyBDMTAuODEyNDkwMywyMy41NzQ4NjggOC4zOTIwNTQyNCwyNiA1LjQwNjI0NTE1LDI2IEMyLjQyMDQ2NjA1LDI2IDAsMjMuNTc0ODY4IDAsMjAuNTgzMzMzMyBDMCwxNy41OTE3OTg2IDIuNDIwNDY2MDUsMTUuMTY2NjY2NyA1LjQwNjI0NTE1LDE1LjE2NjY2NjcgQzguMzkyMDU0MjQsMTUuMTY2NjY2NyAxMC44MTI0OTAzLDE3LjU5MTc5ODYgMTAuODEyNDkwMywyMC41ODMzMzMzIiBpZD0iRmlsbC01NiIgZmlsbD0iI0VDNTQyNCI+PC9wYXRoPgogICAgICAgICAgICA8cGF0aCBkPSJNMjguMjkyNjgyOSwyMC41ODMzMzMzIEMyOC4yOTI2ODI5LDE3LjU5MTc5ODYgMjUuODcyMjE2OCwxNS4xNjY2NjY3IDIyLjg4NjQzNzcsMTUuMTY2NjY2NyBDMTkuOTAwNjU4NywxNS4xNjY2NjY3IDE3LjQ4MDE5MjcsMTcuNTkxNzk4NiAxNy40ODAxOTI3LDIwLjU4MzMzMzMgQzE3LjQ4MDE5MjcsMjMuNTc0ODY4IDE5LjkwMDY1ODcsMjYgMjIuODg2NDM3NywyNiBDMjUuODcyMjE2OCwyNiAyOC4yOTI2ODI5LDIzLjU3NDg2OCAyOC4yOTI2ODI5LDIwLjU4MzMzMzMiIGlkPSJGaWxsLTU3IiBmaWxsPSIjNDRDN0Y0Ij48L3BhdGg+CiAgICAgICAgPC9nPgogICAgPC9nPgo8L3N2Zz4='
@@ -145,3 +147,84 @@ storiesOf('Avatar', module).add('fitted images', () => (
145147
</Stack>
146148
</Example>
147149
))
150+
151+
storiesOf('Avatar', module).add('failure fallbacks', () => (
152+
<>
153+
<Example title="Full fallback - All fail + all missing props">
154+
<Stack>
155+
<Avatar
156+
type="user"
157+
size="xlarge"
158+
email="[email protected]"
159+
initials="failed-initials"
160+
image="some-relative-path-not-found"
161+
/>
162+
<Avatar
163+
type="user"
164+
size="xlarge"
165+
/>
166+
</Stack>
167+
</Example>
168+
<Example title="Missing image fallbacks">
169+
<Stack>
170+
<Avatar
171+
type="user"
172+
size="xlarge"
173+
email="[email protected]"
174+
initials="failed-initials"
175+
/>
176+
<Avatar
177+
type="user"
178+
size="xlarge"
179+
initials="failed-initials"
180+
/>
181+
<Avatar
182+
type="user"
183+
size="xlarge"
184+
email="[email protected]"
185+
/>
186+
<span>All stories with missing <code>image</code> prop used to have an infinite loop issue causing incidents - it's already fixed!</span>
187+
</Stack>
188+
</Example>
189+
<Example title="Missing email">
190+
<Stack>
191+
<Avatar
192+
type="user"
193+
size="xlarge"
194+
initials="failed-initials"
195+
image="some-relative-path-not-found"
196+
/>
197+
<Avatar
198+
type="user"
199+
size="xlarge"
200+
initials="failed-initials"
201+
/>
202+
<Avatar
203+
type="user"
204+
size="xlarge"
205+
image="some-relative-path-not-found"
206+
/>
207+
</Stack>
208+
</Example>
209+
<Example title="Missing initials">
210+
<Stack>
211+
<Avatar
212+
type="user"
213+
size="xlarge"
214+
email="[email protected]"
215+
image="some-relative-path-not-found"
216+
/>
217+
<Avatar
218+
type="user"
219+
size="xlarge"
220+
image="some-relative-path-not-found"
221+
/>
222+
<Avatar
223+
type="user"
224+
size="xlarge"
225+
email="[email protected]"
226+
/>
227+
</Stack>
228+
</Example>
229+
</>
230+
))

‎core/components/atoms/avatar/avatar.tsx‎

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
1-
import * as React from 'react'
2-
import styled from '../../styled'
3-
import { Image } from '../../'
4-
import { colors, misc } from '../../tokens'
5-
import Icon, { __ICONNAMES__ } from '../icon'
6-
import getUserAvatarUrl from '../../_helpers/avatar-url'
7-
import Automation from '../../_helpers/automation-attribute'
8-
import { prependOnceListener } from 'cluster'
1+
import * as React from "react";
2+
3+
import { Image } from "../../";
4+
import Automation from "../../_helpers/automation-attribute";
5+
import getUserAvatarUrl from "../../_helpers/avatar-url";
6+
import styled from "../../styled";
7+
import { colors, misc } from "../../tokens";
8+
import Icon, { __ICONNAMES__ } from "../icon";
99

1010
const iconSizes = {
1111
xsmall: 14,
@@ -33,7 +33,12 @@ const imageForAvatar = (source, handleError) => {
3333
fit="cover"
3434
src={source}
3535
onError={(event) => {
36-
event.target.src = null
36+
try {
37+
event.target.removeAttribute('src')
38+
} catch (e) {
39+
event.target.src = undefined
40+
}
41+
event.target.onerror = undefined
3742
event.target.onError = undefined
3843
handleError(event)
3944
}}
@@ -124,9 +129,9 @@ class Avatar extends React.Component<IAvatarProps, IAvatarState> {
124129

125130
public discardSource(source: 'image' | 'gravatar') {
126131
switch (source) {
127-
case 'image':
132+
case sources.image:
128133
return this.setState({ imageErrored: true })
129-
case 'gravatar':
134+
case sources.gravatar:
130135
return this.setState({ gravatarErrored: true })
131136
default:
132137
return
@@ -137,14 +142,15 @@ class Avatar extends React.Component<IAvatarProps, IAvatarState> {
137142
const { imageErrored, gravatarErrored } = this.state
138143
const { email, initials, icon, image } = this.props
139144

145+
// NOTE: The following order of checks MATTER. They determine the fallback
146+
// precedence when there are any missing props or failed attempts.
147+
// Carefully review all scenarios are covered when changing the
148+
// following lines.
140149
if (icon) { return sources.icon }
141150
if (imageErrored && gravatarErrored) { return sources.fallback }
142-
if (imageErrored || !image) {
143-
if (email || initials) { return sources.gravatar }
144-
return sources.fallback
145-
}
146-
147-
return sources.image
151+
if (image && !imageErrored) { return sources.image }
152+
if ((email || initials) && !gravatarErrored) { return sources.gravatar }
153+
return sources.fallback
148154
}
149155

150156
public render() {

‎internal/test/unit/__snapshots__/avatar.test.tsx.snap‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
// Jest Snapshot v1, https://goo.gl/fbAQLP
2+
3+
exports[`Avatar .getSource() should return "fallback" if both image and gravatar errors to load 1`] = `"fallback"`;
4+
5+
exports[`Avatar .getSource() should return "fallback" if not image nor gravatar property is passed 1`] = `"fallback"`;
6+
7+
exports[`Avatar .getSource() should return "fallback" when gravatar errors to load with email 1`] = `"fallback"`;
8+
9+
exports[`Avatar .getSource() should return "fallback" when gravatar errors to load with initials 1`] = `"fallback"`;
10+
11+
exports[`Avatar .getSource() should return "fallback" when image errors and no gravatar prop is passed 1`] = `"fallback"`;
12+
13+
exports[`Avatar .getSource() should return "gravatar" when both initials and email are passed 1`] = `"gravatar"`;
14+
15+
exports[`Avatar .getSource() should return "gravatar" when email is passed 1`] = `"gravatar"`;
16+
17+
exports[`Avatar .getSource() should return "gravatar" when image errors to load and email is passed 1`] = `"gravatar"`;
18+
19+
exports[`Avatar .getSource() should return "gravatar" when image errors to load and initials are passed 1`] = `"gravatar"`;
20+
21+
exports[`Avatar .getSource() should return "gravatar" when initials are passed 1`] = `"gravatar"`;
22+
23+
exports[`Avatar .getSource() should return "icon" prefered over anything 1`] = `"icon"`;
24+
25+
exports[`Avatar .getSource() should return "image" when image prop passed 1`] = `"image"`;
26+
27+
exports[`Avatar .getSource() should return "image" when image, initials and emails props are passed 1`] = `"image"`;

‎internal/test/unit/avatar.test.tsx‎

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
import { shallow } from "enzyme";
2+
import * as React from "react";
3+
4+
import { Avatar } from "@auth0/cosmos";
5+
6+
const TEST_IMAGE = "https://images.unsplash.com/photo-1507003211169-0a1dd7228f2d?ixlib=rb-0.3.5&q=80&fm=jpg&crop=entropy&cs=tinysrgb&w=200&h=200&fit=crop&crop=faces&s=a72ca28288878f8404a795f39642a46f";
7+
8+
describe('Avatar', () => {
9+
describe('.getSource()', () => {
10+
it('should return "icon" prefered over anything', () => {
11+
const wrapper = shallow<Avatar>(<Avatar size="medium" type="user" icon="plus" initials="dd" email="[email protected]" />)
12+
13+
expect(wrapper.instance().getSource()).toMatchSnapshot()
14+
})
15+
16+
it('should return "fallback" if not image nor gravatar property is passed', () => {
17+
const wrapper = shallow<Avatar>(<Avatar size="medium" type="user" />)
18+
19+
expect(wrapper.instance().getSource()).toMatchSnapshot()
20+
})
21+
22+
it('should return "fallback" if both image and gravatar errors to load', () => {
23+
const wrapper = shallow<Avatar>(<Avatar size="medium" type="user" initials="dd" email="[email protected]" image={TEST_IMAGE} />)
24+
25+
wrapper.setState({
26+
imageErrored: true,
27+
gravatarErrored: true
28+
})
29+
30+
expect(wrapper.instance().getSource()).toMatchSnapshot()
31+
})
32+
33+
it('should return "image" when image prop passed', () => {
34+
const wrapper = shallow<Avatar>(<Avatar size="medium" type="user" initials="dd" email="[email protected]" image={TEST_IMAGE} />)
35+
36+
expect(wrapper.instance().getSource()).toMatchSnapshot()
37+
})
38+
39+
it('should return "image" when image, initials and emails props are passed', () => {
40+
const wrapper = shallow<Avatar>(<Avatar size="medium" type="user" initials="dd" email="[email protected]" image={TEST_IMAGE} />)
41+
42+
expect(wrapper.instance().getSource()).toMatchSnapshot()
43+
})
44+
45+
it('should return "fallback" when image errors and no gravatar prop is passed', () => {
46+
const wrapper = shallow<Avatar>(<Avatar size="medium" type="user" image={TEST_IMAGE} />)
47+
48+
wrapper.setState({
49+
imageErrored: true
50+
})
51+
52+
expect(wrapper.instance().getSource()).toMatchSnapshot()
53+
})
54+
55+
it('should return "gravatar" when image errors to load and email is passed', () => {
56+
const wrapper = shallow<Avatar>(<Avatar size="medium" type="user" image={TEST_IMAGE} email="[email protected]" />)
57+
58+
wrapper.setState({
59+
imageErrored: true
60+
})
61+
62+
expect(wrapper.instance().getSource()).toMatchSnapshot()
63+
})
64+
65+
it('should return "gravatar" when image errors to load and initials are passed', () => {
66+
const wrapper = shallow<Avatar>(<Avatar size="medium" type="user" image={TEST_IMAGE} initials="dd" />)
67+
68+
wrapper.setState({
69+
imageErrored: true
70+
})
71+
72+
expect(wrapper.instance().getSource()).toMatchSnapshot()
73+
})
74+
75+
it('should return "gravatar" when email is passed', () => {
76+
const wrapper = shallow<Avatar>(<Avatar size="medium" type="user" email="[email protected]" />)
77+
78+
expect(wrapper.instance().getSource()).toMatchSnapshot()
79+
})
80+
81+
it('should return "gravatar" when initials are passed', () => {
82+
const wrapper = shallow<Avatar>(<Avatar size="medium" type="user" initials="dd" />)
83+
84+
expect(wrapper.instance().getSource()).toMatchSnapshot()
85+
})
86+
87+
it('should return "gravatar" when both initials and email are passed', () => {
88+
const wrapper = shallow<Avatar>(<Avatar size="medium" type="user" initials="dd" email="[email protected]" />)
89+
90+
expect(wrapper.instance().getSource()).toMatchSnapshot()
91+
})
92+
93+
it('should return "fallback" when gravatar errors to load with email', () => {
94+
const wrapper = shallow<Avatar>(<Avatar size="medium" type="user" email="[email protected]" />)
95+
96+
wrapper.setState({
97+
gravatarErrored: true
98+
})
99+
100+
expect(wrapper.instance().getSource()).toMatchSnapshot()
101+
})
102+
103+
it('should return "fallback" when gravatar errors to load with initials', () => {
104+
const wrapper = shallow<Avatar>(<Avatar size="medium" type="user" initials="dd" />)
105+
106+
wrapper.setState({
107+
gravatarErrored: true
108+
})
109+
110+
expect(wrapper.instance().getSource()).toMatchSnapshot()
111+
})
112+
})
113+
})

0 commit comments

Comments
 (0)