-
Notifications
You must be signed in to change notification settings - Fork 3k
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.
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
Fix 51888 cors errors are displayed for attachments #53407
base: main
Are you sure you want to change the base?
Changes from 15 commits
e8c5c33
b3371ae
28f7a27
433717d
88cf61a
be7f93b
f797f2f
bc0c48b
c7aab35
e5d504d
170e7e8
7ba9f8a
d25f0b0
65fb38c
d9cbf9f
3040f19
cdf9209
bffc444
b9f1240
93a3c05
873dcec
5cbae41
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,17 @@ | ||
import React, {useCallback, useContext, useMemo, useState} from 'react'; | ||
import {withOnyx} from 'react-native-onyx'; | ||
import React, {useCallback, useContext, useEffect, useMemo, useRef, useState} from 'react'; | ||
import FullScreenLoadingIndicator from '@components/FullscreenLoadingIndicator'; | ||
import {useSession} from '@components/OnyxProvider'; | ||
import {isExpiredSession} from '@libs/actions/Session'; | ||
import activateReauthenticator from '@libs/actions/Session/Reauthenticator'; | ||
import CONST from '@src/CONST'; | ||
import ONYXKEYS from '@src/ONYXKEYS'; | ||
import BaseImage from './BaseImage'; | ||
import {ImageBehaviorContext} from './ImageBehaviorContextProvider'; | ||
import type {ImageOnLoadEvent, ImageOnyxProps, ImageOwnProps, ImageProps} from './types'; | ||
import type {ImageOnLoadEvent, ImageProps} from './types'; | ||
|
||
function Image({source: propsSource, isAuthTokenRequired = false, session, onLoad, objectPosition = CONST.IMAGE_OBJECT_POSITION.INITIAL, style, ...forwardedProps}: ImageProps) { | ||
function Image({source: propsSource, isAuthTokenRequired = false, onLoad, objectPosition = CONST.IMAGE_OBJECT_POSITION.INITIAL, style, ...forwardedProps}: ImageProps) { | ||
const [aspectRatio, setAspectRatio] = useState<string | number | null>(null); | ||
const isObjectPositionTop = objectPosition === CONST.IMAGE_OBJECT_POSITION.TOP; | ||
const session = useSession(); | ||
|
||
const {shouldSetAspectRatioInStyle} = useContext(ImageBehaviorContext); | ||
|
||
|
@@ -37,58 +40,115 @@ function Image({source: propsSource, isAuthTokenRequired = false, session, onLoa | |
}, | ||
[onLoad, updateAspectRatio], | ||
); | ||
|
||
// an accepted session is either received less than 60s after the previous | ||
// or is the first valid session since previous session expired | ||
const isAcceptedSession = useCallback((sessionCreationDateDiff: number, sessionCreationDate: number) => { | ||
return sessionCreationDateDiff < 60000 || (sessionCreationDateDiff >= CONST.SESSION_EXPIRATION_TIME_MS && new Date().getTime() - sessionCreationDate < 60000); | ||
}, []); | ||
|
||
/** | ||
* trying to figure out if the current session is expired or fresh from a necessary reauthentication | ||
*/ | ||
const previousSessionAge = useRef<number | undefined>(); | ||
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
const validSessionAge: number | undefined = useMemo(() => { | ||
// for performance gain, the processing is reserved to attachments images only | ||
if (!isAuthTokenRequired) { | ||
return undefined; | ||
} | ||
if (session?.creationDate) { | ||
if (previousSessionAge.current) { | ||
// most likely a reauthentication happens | ||
// but unless the calculated source is different from the previous, the image wont reload | ||
if (isAcceptedSession(session.creationDate - previousSessionAge.current, session.creationDate)) { | ||
return session.creationDate; | ||
} | ||
return previousSessionAge.current; | ||
} | ||
if (isExpiredSession(session.creationDate)) { | ||
return new Date().getTime(); | ||
} | ||
return session.creationDate; | ||
} | ||
return undefined; | ||
}, [session, isAuthTokenRequired, isAcceptedSession]); | ||
useEffect(() => { | ||
if (!isAuthTokenRequired) { | ||
return; | ||
} | ||
previousSessionAge.current = validSessionAge; | ||
}); | ||
|
||
/** | ||
* Check if the image source is a URL - if so the `encryptedAuthToken` is appended | ||
* to the source. | ||
*/ | ||
// source could be a result of require or a number or an object but all are expected so no unsafe-assignment | ||
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
const source = useMemo(() => { | ||
if (typeof propsSource === 'object' && 'uri' in propsSource) { | ||
if (typeof propsSource.uri === 'number') { | ||
return propsSource.uri; | ||
} | ||
const authToken = session?.encryptedAuthToken ?? null; | ||
if (isAuthTokenRequired && authToken) { | ||
return { | ||
...propsSource, | ||
headers: { | ||
[CONST.CHAT_ATTACHMENT_TOKEN_KEY]: authToken, | ||
}, | ||
}; | ||
if (!!session?.creationDate && !isExpiredSession(session.creationDate)) { | ||
// session valid | ||
return { | ||
...propsSource, | ||
headers: { | ||
[CONST.CHAT_ATTACHMENT_TOKEN_KEY]: authToken, | ||
}, | ||
}; | ||
} | ||
if (session) { | ||
activateReauthenticator(session); | ||
} | ||
return undefined; | ||
} | ||
} | ||
return propsSource; | ||
// The session prop is not required, as it causes the image to reload whenever the session changes. For more information, please refer to issue #26034. | ||
// but we still need the image to reload sometimes (exemple : when the current session is expired) | ||
// by forcing a recalculation of the source (which value could indeed change) through the modification of the variable validSessionAge | ||
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps | ||
}, [propsSource, isAuthTokenRequired]); | ||
}, [propsSource, isAuthTokenRequired, validSessionAge]); | ||
useEffect(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please update the comment right above to explain why we should add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok |
||
if (!isAuthTokenRequired || source !== undefined) { | ||
return; | ||
} | ||
forwardedProps?.waitForSession?.(); | ||
}, [source, isAuthTokenRequired, forwardedProps]); | ||
|
||
/** | ||
* If the image fails to load and the object position is top, we should hide the image by setting the opacity to 0. | ||
*/ | ||
const shouldOpacityBeZero = isObjectPositionTop && !aspectRatio; | ||
|
||
if (source === undefined && !!forwardedProps?.waitForSession) { | ||
return undefined; | ||
} | ||
if (source === undefined) { | ||
return <FullScreenLoadingIndicator />; | ||
} | ||
return ( | ||
<BaseImage | ||
// eslint-disable-next-line react/jsx-props-no-spreading | ||
{...forwardedProps} | ||
onLoad={handleLoad} | ||
style={[style, shouldSetAspectRatioInStyle && aspectRatio ? {aspectRatio, height: 'auto'} : {}, shouldOpacityBeZero && {opacity: 0}]} | ||
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
source={source} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a comment to explain why we disable eslint here |
||
/> | ||
); | ||
} | ||
|
||
function imagePropsAreEqual(prevProps: ImageOwnProps, nextProps: ImageOwnProps) { | ||
function imagePropsAreEqual(prevProps: ImageProps, nextProps: ImageProps) { | ||
return prevProps.source === nextProps.source; | ||
} | ||
|
||
const ImageWithOnyx = React.memo( | ||
withOnyx<ImageProps, ImageOnyxProps>({ | ||
session: { | ||
key: ONYXKEYS.SESSION, | ||
}, | ||
})(Image), | ||
imagePropsAreEqual, | ||
); | ||
const ImageWithOnyx = React.memo(Image, imagePropsAreEqual); | ||
|
||
ImageWithOnyx.displayName = 'Image'; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a hook called
usePrevious
, let use it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok i'll check that