-
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 32 commits
e8c5c33
b3371ae
28f7a27
433717d
88cf61a
be7f93b
f797f2f
bc0c48b
c7aab35
e5d504d
170e7e8
7ba9f8a
d25f0b0
65fb38c
d9cbf9f
3040f19
cdf9209
bffc444
b9f1240
93a3c05
873dcec
5cbae41
ae15c5b
ee77c8d
bbe9e2e
80fff80
1c7807a
4174956
528a03c
c66c460
7cb8b5f
fe93bc8
c9dc455
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], | ||||||
); | ||||||
|
||||||
// accepted sessions are sessions of a certain criteria that we think can necessitate a reload of the images | ||||||
// because images sources barely changes unless specific events occur like network issues (offline/online) per example. | ||||||
// Here we target new session received less than 60s after the previous session (that could be from fresh reauthentication, the previous session was not necessarily expired) | ||||||
// or new session after the previous session was expired (based on timestamp gap between the 2 creationDate and the freshness of the new session). | ||||||
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) { | ||||||
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. I don't get this comment, can you explain it to me please? 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. @rlinoz yes we want the session age aspects and related processing to be applied only to images that require authentication (attachments images and receipts) because some other images use the same Image component but are not connected to the cors errors bceause they dont require authentication 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. Thanks, left a suggestion 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.
Suggested change
|
||||||
return undefined; | ||||||
} | ||||||
if (session?.creationDate) { | ||||||
if (previousSessionAge.current) { | ||||||
// Most likely a reauthentication happened, but unless the calculated source is different from the previous, the image won't reload | ||||||
if (isAcceptedSession(session.creationDate - previousSessionAge.current, session.creationDate)) { | ||||||
return session.creationDate; | ||||||
} | ||||||
return previousSessionAge.current; | ||||||
} | ||||||
if (isExpiredSession(session.creationDate)) { | ||||||
// reset the countdown to now so future sessions creationDate can be compared to that time | ||||||
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 | ||||||
const source = useMemo(() => { | ||||||
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.
Suggested change
|
||||||
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)) { | ||||||
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 (example : 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(() => { | ||||||
rlinoz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
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 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. I think we don't need this anymore, right?
Suggested change
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. @rlinoz i am not sure, but i will remove to see |
||||||
/> | ||||||
); | ||||||
} | ||||||
|
||||||
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'; | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,19 +1,12 @@ | ||||||||||||
import type {ImageSource} from 'expo-image'; | ||||||||||||
import type {ImageRequireSource, ImageResizeMode, ImageStyle, ImageURISource, StyleProp} from 'react-native'; | ||||||||||||
import type {OnyxEntry} from 'react-native-onyx'; | ||||||||||||
import type {ValueOf} from 'type-fest'; | ||||||||||||
import type CONST from '@src/CONST'; | ||||||||||||
import type {Session} from '@src/types/onyx'; | ||||||||||||
|
||||||||||||
type ExpoImageSource = ImageSource | number | ImageSource[]; | ||||||||||||
|
||||||||||||
type ImageObjectPosition = ValueOf<typeof CONST.IMAGE_OBJECT_POSITION>; | ||||||||||||
|
||||||||||||
type ImageOnyxProps = { | ||||||||||||
/** Session info for the currently logged in user. */ | ||||||||||||
session: OnyxEntry<Session>; | ||||||||||||
}; | ||||||||||||
|
||||||||||||
type ImageOnLoadEvent = { | ||||||||||||
nativeEvent: { | ||||||||||||
width: number; | ||||||||||||
|
@@ -53,8 +46,14 @@ type ImageOwnProps = BaseImageProps & { | |||||||||||
|
||||||||||||
/** The object position of image */ | ||||||||||||
objectPosition?: ImageObjectPosition; | ||||||||||||
|
||||||||||||
/** Called when the image should wait for a valid session to reload | ||||||||||||
* At the moment this function is called, the image is not in cache anymore | ||||||||||||
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.
Suggested change
|
||||||||||||
* cf https://github.com/Expensify/App/issues/51888 | ||||||||||||
*/ | ||||||||||||
waitForSession?: () => void; | ||||||||||||
}; | ||||||||||||
|
||||||||||||
type ImageProps = ImageOnyxProps & ImageOwnProps; | ||||||||||||
type ImageProps = ImageOwnProps; | ||||||||||||
|
||||||||||||
export type {BaseImageProps, ImageOwnProps, ImageOnyxProps, ImageProps, ExpoImageSource, ImageOnLoadEvent, ImageObjectPosition}; | ||||||||||||
export type {BaseImageProps, ImageOwnProps, ImageProps, ExpoImageSource, ImageOnLoadEvent, ImageObjectPosition}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -234,6 +234,14 @@ function Lightbox({isAuthTokenRequired = false, uri, onScaleChanged: onScaleChan | |
updateContentSize(e); | ||
setLightboxImageLoaded(true); | ||
}} | ||
waitForSession={() => { | ||
// only active lightbox should call this function | ||
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. Can we explain why in the comment please? 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 (!isActive || isFallbackVisible || !isLightboxVisible) { | ||
return; | ||
} | ||
setContentSize(cachedImageDimensions.get(uri)); | ||
setLightboxImageLoaded(false); | ||
}} | ||
/> | ||
</MultiGestureCanvas> | ||
</View> | ||
|
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
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.
Did we give up on using
usePrevious
?