Repository navigation
[HOLD for payment 2024-06-13] [$500] Infinite loading spinner when trying to open a link to an Image that is in a room I have no access to. #23374
Description
Activity
Triggered auto assignment to @greg-schroeder (
Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.Bug0 Triage Checklist (Main S/O)
- This "bug" occurs on a supported platform (ensure
Platformsin OP are ✅) - This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
- This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
- This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
- I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync
- This "bug" occurs on a supported platform (ensure
Proposal
Please re-state the problem that we are trying to solve in this issue.
Infinite loading spinner when trying to open a link to an Image that is in a room I have no access to.
What is the root cause of that problem?
For the attachment that have no access to, app pass empty file.name prop to
AttachmentView.
But inAttachmentView, there is not check whetherfile.nameis empty.
So the infinite loading spinner is shown.What changes do you think we should make in order to solve the problem?
We should add checking whether
file.nameis empty inAttachmentViewcomponent, and if is empty, returnNotFoundView.function AttachmentView(props) { ... if (props.file.name === '') { return <NotFoundPage />; } ...
What alternative solutions did you explore? (Optional)
None.
Reacted by parasharrajatWill review
- changed the title
[-] Infinite loading spinner when trying to open a link to an Image that is in a room I have no access to.[/-][+][$1000] Infinite loading spinner when trying to open a link to an Image that is in a room I have no access to.[/+]on Jul 24, 2023 Job added to Upwork: https://www.upwork.com/jobs/~01440acd5fcf557a24
Triggered auto assignment to @MitchExpensify (
External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.156 remaining items
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
- [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR:
- [@parasharrajat] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
- [@parasharrajat] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
- [@parasharrajat] Determine if we should create a regression test for this bug.
- [@parasharrajat] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
- [@greg-schroeder] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
- [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR: Present fro the start.
- [@parasharrajat] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: NA
- [@parasharrajat] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: Not needed
- [@parasharrajat] Determine if we should create a regression test for this bug. Yes
- [@parasharrajat] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
Regression Test Steps
- Open any chat room and send an image
- Open the image and copy the URL.
- Login as a different user who DOESN'T have access to that room
- Navigate to the link you copied.
- Verify that you see
Not Foundpage for the attachment too.
Do you agree 👍 or 👎 ?
Reacted by Peter Velkov@parasharrajat I don't agree that the PR which introduced the bug was identified correctly. Additionally, you didn't provide an explanation for your conclusion.
The endless loading bug was occurring long before the linked PR. The issue lies within the
AttachmentViewcomponent used in the Carousel items, which is configured to render endless loading when nofallbackSourceis provided.App/src/components/Attachments/AttachmentView/index.tsx
Lines 217 to 218 in 569b7c1
fallbackSourceis optional and there's no default value. In its absence, nothing changes, leading to an endless spinner instead of error handling.The endless loading bug would still occur for any network error.
I believe addressing this underlying issue will provide a more robust solution.
This issue is related to inaccessible reports. So when you don't have access to the report, we shouldn't render the attachment at all. Thus I don't see how providing a fallback to the attachmentView component will help with this issue.
I saw that you added a new route for attachment and thus I thought this is a valid use case to be handled with routing.
But you might be correct, I can be wrong with the root cause. I will try to find the older PRs responsible.
I don't think it was possible to have a link to a specific attachment in the past and while the recent PR may have surfaced the issue, it's more accurate to address the underlying component behavior.
I believe it would be more accurate to render the attachment carousel regardless of the end result. Route parameters match the attachment route, and the user probably expects to see an attachment. In case of an error, like when they don't have access, they'll see the attachment frame and the fallback/error content. The fallback can be further enhanced to display a message based on the network error.
That's what we are showing so far. The attachment modal is shown but instead of an image, it shows
not foundparagraph.Not foundcontent is the fallback here.Updated the checklist to remove the PR responsible. This issue seemed to be present from the start. The app was either crashing or showing a continuous loader when the image was not loaded anyhow.
Processing
Payment summary:
Contributor: @Ahmed-Abdella - $500 - Paid via Upwork
C+: @parasharrajat - $500 - You can make a ND manual requestFiled regression test, closing
Payment requested as per #23374 (comment)
$500 approved for @parasharrajat
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
An error page should appear for the user that he doesn't have access to that attachment because he doesn't have access to its room.
Actual Result:
An infinite spinner for the attachment loading appears, and nothing to tell the user that he can't open that attachment.
After closing it, the user got "Hmm... it's not here You don't have access to this chat'', and if he navigate to the image link again he got the "Uh-oh, something went wrong!" page
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?
Version Number: 1.3.44-0
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Screencast.from.19-07-23.21_28_35.webm
Recording.1317.mp4
Expensify/Expensify Issue URL:
Issue reported by: @Ahmed-Abdella
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1689814964693879
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @greg-schroeder