Fix 51888 cors errors are displayed for attachments #53407
Conversation
|
@hungvu193 should we ask for Design team's help for a better spinner now ? |
|
Not yet, please address the linting. After the codes look good then I will request a review from Design team. |
|
Here's the flow:
|
|
@hungvu193 we need Design to make the choice of the type of image (SVG, GIF, ...) and certainly provide us the image to use based on this video https://github.com/user-attachments/assets/ccd73db5-d00a-49cd-83b2-0b3fb7388577 and the GIF file. Their help will change the code (call to updateAspectRatio, sizing, style...). After all the changes made based on their advice, they review. Thats the work experience i have with them. But as you said we can also go this way #53407 (comment) |
|
@hungvu193 run lint was hanging on my pc. Now we are done with the lint errors, the remaining error is related to the legacy use of withOnyx not our changes. I guess that step should be skipped when deploying on staging. |
Ah no. Once you changed a file that includes |
|
@hungvu193 sorry but that's a whole different issue with its testing and debugging. been there done, done that. We can not take that extra load as we are not even close to be done here. I had the same experience from a previous ticket and the ticket created to make such replacement was a whole issue in itself as it was necessary to avoid regressions. Let's not go that road, i will advise. |
|
We don't create a separate ticket if the changes to the Onyx migration are small. For example, in my previous PR, I also migrated |
|
@hungvu193 i propose we reconsider that possibility once we are done with the main issue |
|
What's the main issue? The |
|
@hungvu193 dont forget that Image is a central component highly used in every screen of the code, so this "simple" change will require a lot of testing for possible regressions. Why not deal with issues one after the other ? |
|
@hungvu193 Many have made changes and PRs on this file before us and faced that Lint error/warning. This "simple" component could be at high risks of regressions. I will advise in this case to not do anything instead of breaking something |
src/CONST.ts
Outdated
| @@ -1445,6 +1445,8 @@ const CONST = { | |||
| UNKNOWN: 'unknown', | |||
| }, | |||
| }, | |||
| // the number of hours for an idle session to expire | |||
| SESSIONS_MAXIDLE_NB_HOURS: 2, | |||
There was a problem hiding this comment.
Can we use milliseconds instead?
src/components/Image/index.tsx
Outdated
| } | ||
| return previousSessionAge.current; | ||
| } | ||
| if (Math.abs(new Date().getTime() - session.creationDate) >= CONST.SESSIONS_MAXIDLE_NB_HOURS * 3600000) { |
There was a problem hiding this comment.
If we used milliseconds for CONST.SESSIONS_MAXIDLE_NB_HOURS we can do this instead:
| if (Math.abs(new Date().getTime() - session.creationDate) >= CONST.SESSIONS_MAXIDLE_NB_HOURS * 3600000) { | |
| if (Math.abs(new Date().getTime() - session.creationDate) >= CONST.SESSIONS_MAXIDLE_NB_HOURS) { |
| /** | ||
| * trying to figure out if the current session is expired or fresh from a necessary reauthentication | ||
| */ | ||
| const previousSessionAge = useRef<number | undefined>(); |
There was a problem hiding this comment.
We have a hook called usePrevious, let use it.
There was a problem hiding this comment.
ok i'll check that
There was a problem hiding this comment.
Did we give up on using usePrevious?
src/components/Image/index.tsx
Outdated
| useEffect(() => { | ||
| previousSessionAge.current = validSessionAge; | ||
| }); |
There was a problem hiding this comment.
After using usePrevious we can remove this block:
| useEffect(() => { | |
| previousSessionAge.current = validSessionAge; | |
| }); |
src/components/Image/index.tsx
Outdated
| [CONST.CHAT_ATTACHMENT_TOKEN_KEY]: authToken, | ||
| }, | ||
| }; | ||
| if (!!session?.creationDate && new Date().getTime() - session.creationDate < CONST.SESSIONS_MAXIDLE_NB_HOURS * 3600000) { |
There was a problem hiding this comment.
Can we create a function called isValidSessionCreationDate to reuse this logic?
ie:
function isValidSessionCreationDate() {
return !!session?.creationDate && (new Date().getTime() - session.creationDate) > CONST.SESSIONS_MAXIDLE_NB_HOURS
}There was a problem hiding this comment.
i'll see how can isolate that logic in a function with a relevant name, good idea
src/components/Image/index.tsx
Outdated
| if (Math.abs(previousSessionAge.current - session.creationDate) < 60000) { | ||
| return session.creationDate; | ||
| } |
There was a problem hiding this comment.
Can you explain why we have this condition?
There was a problem hiding this comment.
we expect a reauthenticate to happen in less than 60s if the current session was expired. I have made the tests. so the new valid session will be newer than 60s after
src/components/Image/index.tsx
Outdated
| @@ -72,6 +102,7 @@ function Image({source: propsSource, isAuthTokenRequired = false, session, onLoa | |||
| {...forwardedProps} | |||
| onLoad={handleLoad} | |||
| style={[style, shouldSetAspectRatioInStyle && aspectRatio ? {aspectRatio, height: 'auto'} : {}, shouldOpacityBeZero && {opacity: 0}]} | |||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | |||
There was a problem hiding this comment.
Please add a comment to explain why we disable eslint here
We still need to do it anyway. PR can't be merged if all the tests aren't passed. That's mandatory. |
How did previous PRs end up in main then ? I really think we should avoid this as we can never do enough testing for regressions imho |
Changes from previous PRs were added when we didn't deprecate |
|
@hungvu193 lets change it after we are done with the main issue, if you insist on it. I can't put my focus on that right now as i must start testing for regressions right away once we change it. |
|
@hungvu193 i will upload the web test video as you can see the sizing of the image cause the attachments not to render properly (we cant just use any gif from the web). So the code will be adjusted once we have the definitive image from Design. I think we can have Design team's help based on the web test and the other envs are not necessary as they will be reviewed anyway once we have the definitive image demo_with_2H_expired_token.mp4 |
|
The fact that we display image based on its size. I'm thinking about the way we should keep image loading until it has valid session. Wdyt? |
|
So There's no image/gif placeholder needed. Instead we improve the loading conditions of image |
the problem is the final source of the image is not known yet at that step, it will be recalculated once we have a valid session, so we cannot anticipate the loading unless you're thinking of some other way for it |
|
@hungvu193 i did some updates based on your review but it didnt use the useprevious hook yet (i will test it some more). |
|
I think for now that's fine to keep image resize like that. Let's complete the author checklist and mark this PR as ready for review. I took a few tests, everything seems working fine. Let's finish the PR phrase so I'll request a review from Design team 😄 |
|
So I abandoned my tab for hours and when I'm back I got this issue. Screen.Recording.2024-12-05.at.15.40.06.mov |
|
As I checked the log, ReAuthenticate seems to never get called during that time, so session was invalid and the loading was showed forever |
yes we need to fix it in the caroussel also. I'll work on it. |
|
@hungvu193 we will be using a reauthenticator which will be a singleton object called when the spinner is returned as source for the image. The current session will be send as parameter. It will listen to network and session onyx keys so it wont do anything if offline and will deactivate once it receive a session from Onyx. Once activated (only once) it will expect a session from Onyx in the next 10s (preferred) or 15s and if it doesnt receive a new session it will ask for reauthentication (only once with no retry). Wdyt ? i'll implement it |
|
@hungvu193 but normally the notification pusher triggers reauthentifications (if necessary every 5 seconds) and we shouldnt need a reauthenticator triggers edit* |
|
I will do some testings about the caroussel display |
|
@hungvu193 i have implemented the reauthenticator (still i dont know what you think of the idea) https://github.com/Kalydosos/App/blob/fix-51888-cors-errors-are-displayed-for-attachments/src/libs/actions/Session/Reauthenticator.ts . I let the comments in the code to help you test it out. I set the session expiration time to 5mn for testing. You can then see how it works for the image in the chat reauthenticate_for_thread_images.mp4and then for the carousel reauthenticator_demo.mp4the point is now to make sure it is used when it is really necessary and maybe we could shorten the wait time to 7s or 8s |
…ttps://github.com/Kalydosos/App into fix-51888-cors-errors-are-displayed-for-attachments
…d-for-attachments
|
@hungvu193 it's all good now except the translations #53407 (comment) . I redo all the videos (for the nth time 😃 ), hope it's all setted ! |
|
Let's wait for confirmation from Internal Engineer in our ongoing discussion |
@hungvu193 this is internal tool, it has no impact on the fix. I was stating that calling it "Invalidate session delayed" could be misleading as any tester could think that it's a delayed version of the "Invalidate session" testtool, which it's not. @rlinoz gave us a go for the tool and suggested a name. He never meant we have to call it as such by all costs imho, please let me know if i'm wrong. I explained the choice of the name here #53407 (comment) yesterday |
| showContextMenuForReport( | ||
| event, | ||
| anchor, | ||
| report?.reportID ?? '-1', |
There was a problem hiding this comment.
| report?.reportID ?? '-1', | |
| report?.reportID ?? CONST.DEFAULT_NUMBER_ID, |
There was a problem hiding this comment.
check the new commit on that point
There was a problem hiding this comment.
One suggestion for you that you need to have meaningful commit message instead repeating fix cors errors on attachments.
|
@hungvu193 i updated as we discuss, videos also are updated |
|
Thanks. The only pending here is the translation confirmation. |
still an internal tool so i think we shouldn't wait too long if there is no translation provided |
|
Hey @rlinoz This is ready for your final review! |
|
@Kalydosos Can you resolve the conflicts in the meantime? Ty |
src/components/Image/index.tsx
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
Please explain why and what is an acceptedSession in the comment
There was a problem hiding this comment.
@rlinoz 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).
| /** | ||
| * trying to figure out if the current session is expired or fresh from a necessary reauthentication | ||
| */ | ||
| const previousSessionAge = useRef<number | undefined>(); |
There was a problem hiding this comment.
Did we give up on using usePrevious?
src/components/Image/index.tsx
Outdated
| 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 |
There was a problem hiding this comment.
I don't get this comment, can you explain it to me please?
There was a problem hiding this comment.
@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
| @@ -111,6 +111,12 @@ function ImageWithSizeCalculation({url, altText, style, onMeasure, onLoadFailure | |||
| }} | |||
| onError={onError} | |||
| onLoad={imageLoadedSuccessfully} | |||
| waitForSession={() => { | |||
| // at the moment this function is called the image is not in cache anymore | |||
There was a problem hiding this comment.
| // at the moment this function is called the image is not in cache anymore | |
| // At the moment this function is called the image is not in cache anymore |
| @@ -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.
Can we explain why in the comment please?
src/types/onyx/Session.ts
Outdated
| @@ -36,6 +36,9 @@ type Session = { | |||
|
|
|||
| /** User signed in with short lived token */ | |||
| signedInWithShortLivedAuthToken?: boolean; | |||
|
|
|||
| /** timestamp of the session creation date */ | |||
There was a problem hiding this comment.
| /** timestamp of the session creation date */ | |
| /** Timestamp of the session creation date */ |
src/libs/actions/Session/index.ts
Outdated
| } | ||
|
|
||
| /** | ||
| * Send an expired session to FE and invalidate the session in the BE. Action is delayed for 15s |
There was a problem hiding this comment.
I think this is not calling the BE at all, is it?
There was a problem hiding this comment.
no it doesnt call the BE but the next request to the BE will send the "pizza" token, i will rephrase to "Send an expired session to FE and invalidate the session in the BE perspective"
| * @param session the current session | ||
| * @returns | ||
| */ | ||
| function activate(session: Session) { |
There was a problem hiding this comment.
We already have reauthentication mechanisms throughout the app, can you help me understand why we can't use those?
There was a problem hiding this comment.
yes we are using the core (re)Authentication feature but we need more control over the context of the use of that feature, meaning only one Image component can ask for reauthentication (in a singleton mode) and we only use that feature when there hasnt been any reauthentication for 3.5s since we detected that the current session is expired (particulary useful in the carousel)
There was a problem hiding this comment.
From what I can tell Reuathenticate.reauthenticate also ensures that we make a single auth request, I also would like to avoid all these setTimeout calls honestly
Co-authored-by: Rodrigo Lino da Costa <[email protected]>
Co-authored-by: Rodrigo Lino da Costa <[email protected]>
Co-authored-by: Rodrigo Lino da Costa <[email protected]>
…ttps://github.com/Kalydosos/App into fix-51888-cors-errors-are-displayed-for-attachments
src/components/Image/index.tsx
Outdated
| 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 |
There was a problem hiding this comment.
| // for performance gain, the processing is reserved to attachments images only | |
| // Authentication is required only for certain types of images (attachments and receipts), so let's only calculate the session age for those |
src/components/Image/index.tsx
Outdated
| 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 |
src/components/Image/index.tsx
Outdated
| /** | ||
| * 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 |
There was a problem hiding this comment.
| // source could be a result of require or a number or an object but all are expected so no unsafe-assignment |
src/components/Image/index.tsx
Outdated
| 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 |
There was a problem hiding this comment.
I think we don't need this anymore, right?
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment |
There was a problem hiding this comment.
@rlinoz i am not sure, but i will remove to see
src/components/Image/types.ts
Outdated
| /** 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.
| /** 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 | |
| /** | |
| * 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 |
| * @param session the current session | ||
| * @returns | ||
| */ | ||
| function activate(session: Session) { |
There was a problem hiding this comment.
From what I can tell Reuathenticate.reauthenticate also ensures that we make a single auth request, I also would like to avoid all these setTimeout calls honestly
|
@rlinoz i tried not using a timeout for the reauthentication and we had the reauthentication calls overlapping and previous images reload requests where canceled and then retrieved with the 2nd session from the 2nd reauthentication (we are dealing with milliseconds calls indeed)
|
|
@rlinoz the Reauthenticator.ts was only needed in fact for when the user clicks on the image and display the carousel but the session was expired (the "Action Performed" of the ticket and also this test case #53407 (comment)). At that moment, no "natural" reauthentication was triggered so we have to call it ourselves. But we needed to be sure we were in the carousel but using attachmentCarouselPagerContext?.pagerRef was not reliable where the carousel is accessed from deep linking so we implemented Reauthenticator. But to avoid the overlap of reauthentication calls (in my previous comment) we set up a timeout which is only useful when in a chat thread per example |
Explanation of Change
Fixed Issues
$ 51888
PROPOSAL: #51888 (comment)
Tests
Test 1 steps
Test 2 steps
Note : on native platforms, the troubleshooting test tool "Authentication Status/Invalidate with delay" can be used to simulate expiring the session in the next 15 seconds when clicked.
Test 3 steps
Note : on native platforms, the troubleshooting test tool "Authentication Status/Invalidate with delay can be used to simulate expiring the session in the next 15 seconds when clicked.
Offline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android_native.mp4
Android: mWeb Chrome
android_mweb.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_mweb_safari.mp4
MacOS: Chrome / Safari
ios_web_safari.mp4
MacOS: Desktop
macos_desktop.mp4