Fix 51888 cors errors are displayed for attachments#53407
Fix 51888 cors errors are displayed for attachments#53407rlinoz merged 35 commits intoExpensify:mainfrom
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
| }, | ||
| }, | ||
| // 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?
| 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
| {...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 |
Co-authored-by: Rodrigo Lino da Costa <5201282+rlinoz@users.noreply.github.com>
Co-authored-by: Rodrigo Lino da Costa <5201282+rlinoz@users.noreply.github.com>
…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 |
| * @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 |
|
Did you just try to remove the timeouts? I am suggesting we use this reauthenticate App/src/libs/Middleware/Reauthentication.ts Lines 18 to 20 in c7aab35 which should handle concurrent calls |
|
@rlinoz yes i considered using the middleware Reauthentication but it is a component with its own logic and a lot happens before and after its call to the reauthenticate function. I am not so sure about directly calling the reauthenticate function then. And as attachments images and receipts are loaded one after the other, maybe some of the direct calls will hit after a previous reauthentication was done (or while it's being cleaned up) and create then multiple reauthentications if per example the report has many attachments and receipts. But i will give your suggestion a try and come up with test cases to clear or confirm the doubts |
|
@rlinoz as i was suspecting we indeed have many authenticate requests in this quick test below, i suspect we could also have other issues in the long run maybe setting the middleware in an inconsistent state per example. About the timeout in the code i must add that it will be called only once per session expired situation, only when the session is expired. Also the middleware use a timeout through RequestThrottle so i think i will be difficult not to use one if we wish to control the behavior and the context of the calls |
…d-for-attachments
|
Right, thanks for trying! I will leave a review on the Reauthenticator then 🙇 |
|
@rlinoz changes done 👍 I ran a few extra tests and the result for "simulate failing requests" make me think it will be a good thing on UX standpoint to catch any error in the line below |
|
@rlinoz thanks for the help 👍 |
No, he was assigned because @Kalydosos didnt mention the correct PR when marking it as ready. I think you can go ahead and merge it |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/rlinoz in version: 9.0.83-0 🚀
|
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.83-5 🚀
|
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.83-5 🚀
|
| <TestToolRow title={translate('initialSettingsPage.troubleshoot.authenticationStatus')}> | ||
| <Button | ||
| small | ||
| text={translate('initialSettingsPage.troubleshoot.invalidateWithDelay')} | ||
| onPress={() => Session.expireSessionWithDelay()} | ||
| /> | ||
| </TestToolRow> |
There was a problem hiding this comment.
We need to adjust the existing style when introducing this test tool (because the button contains a long text), otherwise, the button text will be clipped. More details here
There was a problem hiding this comment.
I suggested a shorter but meaningful title but i had to follow the C+ lead #53407 (comment). Thanks for the information
Kalydosos
deleted the
fix-51888-cors-errors-are-displayed-for-attachments
branch
| return undefined; | ||
| } | ||
| if (source === undefined) { | ||
| return <FullScreenLoadingIndicator />; |
There was a problem hiding this comment.
Loading indicator has large size by default, thus causing this one #61073.
7 participants
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