-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[$500] Attachments - Image preview did not load immediately after being received #51699
Comments
Triggered auto assignment to @strepanier03 ( |
This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989 |
I witnessed this happening in a FullStory session for https://github.com/Expensify/Expensify/issues/439858. There isn't much for reproduction steps, they were trying to upload screenshots to concierge. |
Job added to Upwork: https://www.upwork.com/jobs/~021851730798154046033 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @abdulrahuman5196 ( |
Trying to nip this in the bud a bit since a few of us have experienced the bug but can't reliably reproduce. Posted in #expensify-open-source too |
Precondition: Reproduction steps 1
Video51699.1.webmReproduction steps 2
Video51699.2.webmReproduction steps 3(same as #44445 (comment))
Video51699.mov |
Thx @CyberAndrii . |
Just happened again to me. Vague repro steps
Posted in #contributor-plus for bonus 👀 too |
Upwork job price has been updated to $500 |
Upwork job price has been updated to $250 |
Upwork job price has been updated to $500 |
Sorry y'all, a lil behind on this one. I removed 'reproducible steps' in the title and bumped the job price to $500. I'm assuming the reproduction steps would help someone submit a proposal. @CyberAndrii , did you also want to submit a proposal to fix the bug? Also, can you please accept the job and reply here once you have? https://www.upwork.com/jobs/~021851730798154046033 |
Edited by proposal-police: This proposal was edited at 2024-11-26 06:51:49 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.Image preview did not load immediately after being received and CORS error is displayed What is the root cause of that problem?There are in fact 2 causes to this issue cause 1 Some attachments are seen as of external source
they are requested without authentication token from the following lines App/src/components/Image/index.tsx Lines 50 to 57 in f2e295f
The BE then send a redirect as exitTo url but which is requested without the proper "Access-Control-Allow-Origin" header. it doesnt reproduce all the time because sometimes the image is in the cache and is not requested from the BE video_cause1.mp4cause 2 The second cause is that the images are, at the very beginning of the screen loading (deeplinking to report), requested with an expired authentication token even though in most cases a reauthentication have been made. That should not happen but in fact the images loading were disconnected from direct session changes because of a bug in the past #26034 App/src/components/Image/index.tsx Lines 60 to 62 in f2e295f
Then even though a reauthentication happens like in the images below (after reconnectApp gets a 407 from the BE) some of the images are firstly requested with an expired authentication token These 2 causes are well sumed up in the video below requetes_302_200_302.mp4What changes do you think we should make in order to solve the problem?For cause 1
into
For cause 2 in type Session.tsx https://github.com/Expensify/App/blob/f2e295fd174c87f7a516800c22028b637746b22c/src/types/onyx/Session.ts we added we change the following lines App/src/components/Image/index.tsx Lines 44 to 62 in f2e295f
into we force useMemo to recalculate and rerender when the session is outdated and we cover cases like the first outdated session without age and the first still valid session without age. Based on the experience working on this issue, we think that any update of the session by Onyx in less than 60s after the previous update may be related to a reauthentication and thus we must recalculate the image source and rerender the image we also change https://github.com/Expensify/App/blob/f2e295fd174c87f7a516800c22028b637746b22c/src/libs/actions/Session/updateSessionAuthTokens.ts into this
and App/src/libs/E2E/actions/e2eLogin.ts Lines 54 to 66 in f2e295f
into in Session/index.tsx https://github.com/Expensify/App/blob/f2e295fd174c87f7a516800c22028b637746b22c/src/libs/actions/Session/index.ts we added RESULTIt works like a charm, the attachments are properly displayed in both cases . video before video after video testing the 2 Hours expired token What alternative solutions did you explore? (Optional)None |
@mallenexpensify I'm submitting a proposal above but it is just for the solution. For the repro steps it's totally inspired from @CyberAndrii proposal. So i think @CyberAndrii and i could share the bounty if you agree, him for the repro steps and me for the solution |
To reproduce the bug for cause1 (in my proposal), here is a new step (also inspired from @CyberAndrii ) directly copy the markdown the attachment lose its data-expensify-source and then the bug occurs. Check the videos of the repro below repro_causeA_demo1_2.mp4repro_causeA_demo2_2.mp4for cause 2 (in my proposal) we will use @CyberAndrii repro technique but we will a an age to the session as you can see in the video (here my solution is implemented, so there is a spinner instead of the gray icon while waiting for a valid session), we will send
in the console to mimick the 2 hours expired session. It can be used both to reproduce the issue and also to test the solution of the PR demo_with_2H_expired_token.mp4 |
in fact we have been working on this issue in two different tickets, one for the repro steps (this ticket) and on another which @CyberAndrii also know of, but the other ticket have to revised the repro steps and it's not moving forward. So if my proposal is accepted in this ticket, i'll let know the team in the other ticket and they could close it as it is the same issue at the source |
@mallenexpensify @stitesExpensify @abdulrahuman5196 this issue is now 4 weeks old, please consider:
Thanks! |
There's a similar issue open that will have the same fix. @hungvu193 @abdulrahuman5196 you will need to decide who will review the proposals. @mallenexpensify I have accepted the offer for repro steps, thanks. |
Contributor: @CyberAndrii paid $250 via Upwork for documenting reproducible steps. Commented on the other issue, 🤞 we can get this or that one closed so we can focus on one fix (to rule them all!!!!) |
@mallenexpensify Seems @hungvu193 is the C+ in the other issue which has used the repro steps here to move forward with the proposal. |
@abdulrahuman5196 , I feel like $250 is fair here for C+ because that was the original price when you did the work and cuz there's no PR to review. Do you agree? Feel free to follow the PR that's raised from the other issue, if you want and think you might be able to provide feedback (it's not necessary though). |
Issue not reproducible during KI retests. (Third week) |
I do agree @mallenexpensify . If you are good as well, could you kindly add the payment summary. Let me know if anything else is required here as well. |
Contributor+: @abdulrahuman5196 due $250 via NewDot Thanks! (I think we're good close here, follow the other issue if ya want) |
$250 approved for @abdulrahuman5196 |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: 9.0.54-11
Reproducible in staging?: Yes
Reproducible in production?: Unknown
Email or phone of affected tester (no customers): [email protected]
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: @mallenexpensify
Slack conversation: #quality
Deliverable
The deliverable is to provide reliable reproduction steps for the bug. - Follow the Propose a solution for a job process to submit the steps for review”. Inc. preconditions and additional details if they’re helpful. (ie. require x beta to be enabled, must be on a Collect plan)
Action Performed:
Precondition: Both user A and B are online at the same time. User A is logged in on Desktop app.
Expected Result:
Image preview is displayed in the conversation history
Actual Result:
Grey box shows, image doesn't auto-load (it does load later)
Workaround:
Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
onyx data image not showing.txt
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @mallenexpensifyThe text was updated successfully, but these errors were encountered: