Navigating to a chat you can't access results in a blank screen

[marcaaron]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Got an email for a report comment and clicked the link
2. Was logged into E.cash with an account that could not access said report
3. Landed on an empty report

Expected Result:

Get redirected to a 404 page or something.

Actual Result:

LHN is visible but report view displays a white screen with no additional information

Workaround:

N/A

Platform:

All

Version Number: version: "1.0.74-0"
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:

View all open jobs on Upwork

[MelvinBot]

Triggered auto assignment to @bfitzexpensify (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[dklymenk]

Hello, if you decide to make this external I have a proposal.

It should be as simple as refactoring ReportView to render the error instead of ReportActionsView and ReportActionCompose, if there is no reportID for requested report in local storage.

const ReportView = ({reportID, report, session}) => {
    if (!report.reportID) {
        return (
            <View style={[styles.flex1, styles.alignItemsCenter, styles.justifyContentCenter]}>
                <Text style={[styles.h1]}>404</Text>
            </View>
        );
    }
    return (
        <View nativeID={CONST.REPORT.DROP_NATIVE_ID} key={reportID} style={[styles.flex1, styles.justifyContentEnd]}>
            <ReportActionsView reportID={reportID} />

            {session.shouldShowComposeInput && (
                <SwipeableView onSwipeDown={() => Keyboard.dismiss()}>
                    <ReportActionCompose
                        onSubmit={text => addAction(reportID, text)}
                        reportID={reportID}
                    />
                </SwipeableView>
            )}
            <KeyboardSpacer />
        </View>
    );
};

and also map report to props via withOnyx:

export default withOnyx({
    report: {
        key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
    },
    session: {
        key: ONYXKEYS.SESSION,
    },
})(ReportView);

also adjust proptypes accordingly.

It will look like this:

I'll obviously adjust the looks and text to whatever is required by your design team.

The benefit of doing it like this over redirecting to some different page is the fact that user can use sidebar to switch to a chat he has access to and start chatting right away. Or logout and login with his second account to open the chat he didn't have access to with his previous account. Basically, all the UI is available right away, thus better UX.

[aliabbasmalik8]

PROPOSAL

We can throw an error if we have no access to that chat and catch an error in ErrorBoundary as we already using ErrorBoundary. right now there is no fallback UI so we can show a simple and appropriate message for now.

Thanks

[MelvinBot]

Triggered auto assignment to @Beamanator (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[Beamanator]

Good find, looks like it's easily reproducible by just navigating to https://staging.expensify.cash/r/25 - a.k.a. a report you don't have access to. Marking External 👍

[MelvinBot]

Triggered auto assignment to @kadiealexander (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[kadiealexander]

https://www.upwork.com/jobs/~018b221693212487f0

[MelvinBot]

Triggered auto assignment to @marcaaron (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[mygit1313]

Hi

Posting my comment for this job on Upwork https://www.upwork.com/jobs/React-Native-Fix-issue-where-link-click-results-blank-screen-3476_~018b221693212487f0/

My Profile is https://www.upwork.com/freelancers/~014125b829d935f4b8

My comment: I think you must skip some thing, Your component renders before your response comes, you must have to do the following things:

Please check the server response, if everything is fine, Then apply conditional statement at the response, so that response is visible after the component renders and if there is no data then inside condition you can add 404 or some component that represent 404.

I greatly look forward to a positive response.

Thanks
Anshu

[marcaaron]

It should be as simple as refactoring ReportView to render the error instead of ReportActionsView and ReportActionCompose, if there is no reportID for requested report in local storage.

@dklymenk How will we differentiate between a report that "hasn't yet loaded" and one that we are "unable to access". There is a subtle difference and I think the solution you are providing might lead to a brief flash of a 404 while we are waiting for a report to load.

We can throw an error if we have no access to that chat

@aliabbasmalik8 This proposal could work, but is missing how we'd determine the user has no access to the chat

@mygit1313 This proposal could be on the right track but needs to be more specific

Ultimately, I'm not sure if we want to redirect to a 404 page or just use an error growl and redirect the user to a report they can access (like 1:1 w/ Concierge), but maybe @Expensify/design has ideas. I know we had previously discussed showing a 404 page (which actually still exists here but is unused).

As for detecting whether a user truly has access to a report or not. I'm not sure whether it's enough to look and see if it exists in storage. There could be any number of reasons why the report isn't there (and a user could still be able to access it).

[dklymenk]

@dklymenk How will we differentiate between a report that "hasn't yet loaded" and one that we are "unable to access". There is a subtle difference and I think the solution you are providing might lead to a brief flash of a 404 while we are waiting for a report to load.

In my testing this was not the case. I think I specifically did it in that exact component because ReportView is not rendered if data is not loaded. See ReportScreen.js and my video below. There is no 404 flashing.

screen.mp4

As for detecting whether a user truly has access to a report or not. I'm not sure whether it's enough to look and see if it exists in storage. There could be any number of reasons why the report isn't there (and a user could still be able to access it).

If I understand correctly, only way to get access to a report that is not in your initial list of reports (the one you get from BE via api?command=Get endpoint with returnValueList: chatList), is to create new one by starting a chat with a new person or a group. I actually tried it with my proposal and there was an error caused by the fact that for a brief moment there is no report with new ID on localstorage. If I replace if (!report.reportID) { with if (report && !report.reportID) { the problem is resolved. You will only see 404 error if report_<id> is in local storage and it doesn't have required data from BE (I'm testing for reportID in my proposal).

[marcaaron]

In my testing this was not the case. I think I specifically did it in that exact component because ReportView is not rendered if data is not loaded. See ReportScreen.js and my video below. There is no 404 flashing.

I'll ask this another way :)

If for some reason the user should have access to the report, but it just hasn't loaded yet - then we are showing them a 404 in that case. To test this you would need to remove the report you are trying to access from localStorage (which simulates the situation where you have not yet accessed it - but should have access) then try navigating to it directly via a deep link or browser url.

If I understand correctly, only way to get access to a report that is not in your initial list of reports (the one you get from BE via api?command=Get endpoint with returnValueList: chatList), is to create new one by starting a chat with a new person or a group.

Reports can also be shared with you, but you are describing how to create a new chat. To tell if you have access to a chat we could use something like https://www.expensify.com/api?Get=reportStuff?reportIDList=12345 and the response would look like:

{
    "jsonCode": 404,
    "message": "Auth Get returned an error",
    "codeRevision": "0000000000000000000000000000000000000000",
    "requestID": "EUfCx4"
}

We try to load all the chats you have access to when the app first inits which calls that API (see code here). But the absence of a report doesn't necessarily mean that a user has no access to it. It could still be loading.

I actually tried it with my proposal and there was an error caused by the fact that for a brief moment there is no report with new ID on localstorage. If I replace if (!report.reportID) { with if (report && !report.reportID) { the problem is resolved. You will only see 404 error if report_ is in local storage and it doesn't have required data from BE (I'm testing for reportID in my proposal).

Sorry, what exactly did you try? It makes sense to me that you'd see an error, but not really sure how this connects to your previous point.

Thanks!

[dklymenk]

If for some reason the user should have access to the report, but it just hasn't loaded yet - then we are showing them a 404 in that case. To test this you would need to remove the report you are trying to access from localStorage (which simulates the situation where you have not yet accessed it - but should have access) then try navigating to it directly via a deep link or browser url.

You're right. Deleting the report from localstorage manually and refreshing the page leads to 404 with my proposal. Still, I don't understand how to reproduce this state without touching local storage. After logging out and logging in I don't see 404 during or after loading.

Sorry, what exactly did you try? It makes sense to me that you'd see an error, but not really sure how this connects to your previous point.

I mean. I've tried to create a new chat with a person I've never had a chat with, and there was no 404 message.

---

Anyway, I think I see what you're getting at. Even if it works right for me right now, it might not work down the line since even some minor change to how we store, handle or even fetch the data could lead to 404 flashing. Using secondary properties of data objects to show an error message is not a good practice anyway.

[marcaaron]

Still, I don't understand how to reproduce this state without touching local storage.

To be honest, it's just a hunch. I'm not sure how likely it would be in ideal networking scenarios, but the case I'm thinking of is...

• Network request causes some newly shared report to load very slow (can possibly simulate this with some heavy network throttling)
• User navigates to report via device notification and sees 404 instead of a loading spinner or something to indicate the report is being fetched

[aliabbasmalik8]

We can do this using ErrorBoundary as I mentioned in this PROPOSAL

for this case, we can throw an error like this way

if (report && !report.reportID) {
// also need to check this reportId  exist at backend before throwing error like this
// `API.isReportExist` this API not exist just use to explain the proposal
     API.isReportExist(reportId).then(({isReportExist}) => {
       !isReportExist && throw new Error('ReportID not found ........');
     })
  }

at this point https://github.com/Expensify/Expensify.cash/blob/57f557cd31198a631305b617c374f2418eb5e9fa/src/pages/home/report/ReportView.js#L33

and catch in ErrorBoundary. we also need to update ErrorBoundary to display an error message.
You can see in the below picture

NOTE

1. we can store all types of error messages in some const array and throw accordingly by getting from the array.
2. API.isReportExist this API not exist just use to explain the proposal
3. In Error boundary we can provide an appropriate message with the home page URL.

Error Boundaries Example

[marcaaron]

@aliabbasmalik8 Not sure we need to use the error boundary. If we are only using it so that we can catch a thrown error then it could make more sense to redirect to a 404 screen, show a quick error growl, or a confirm modal that redirects you back to the site root

@Expensify/design thoughts on what we should prefer here?

As for creating something like isReportExists... as mentioned here, I think we can use Get&returnValueList=reportStuff. It seems incorrect to call this inside the render() method.

We could perform this in componentDidMount() instead since in that case:

1. The report would not exist and the screen would be blank anyway
2. We can attempt to fetch the report if it's not accessible for some reason
3. If we fail and get a 404 the user must not be able to access it and we can take some other action

[shawnborton]

I am having a little difficulty following all of the conversation, but I think in general I like the idea of just punting the user to a chat they can see (maybe it was the last chat the user was viewing) and then launching either a growl or a little confirm modal that says the chat can't be found. Is that what you were thinking Marc?

[marcaaron]

I think in general I like the idea of just punting the user to a chat they can see (maybe it was the last chat the user was viewing) and then launching either a growl or a little confirm modal that says the chat can't be found. Is that what you were thinking Marc?

That works for me! So we would essentially need to:

1. Redirect the user back to the site root / - which should load a chat that they can see
2. Show a growl to let them know the previous report they are trying to access can't be accessed

[rdjuric]

Looks like the flow should be something like:

1. In componentDidMount of ReportActionsView check if the local report object has a reportID (we optimistically save some data locally to a visited report, so checking if it's an empty doesn't work)
2. If it doesn't, call Get=reportStuff with reportIDList=[this.props.reportID].
3. If the API returns 404 or an empty report object, show a growl and redirect the user to /
4. If the API returns a valid report object, it means that we have access to that report but it isn't in our localStorage yet. So we can call fetchChatReportsByIDs with our reportID and it will get the info associated with that report and merge it locally.

Does this looks OK @marcaaron?

[marcaaron]

Looks mostly like what I had in mind @rdjuric, but now thinking we may just want to call fetchChatReportsByIDs() (which calls Get&rvl=reportStuff) so that all the report data will be loaded in the case that it does succeed.

e.g. if there's only one reportID passed to fetchChatReportsByIDs() and we can't access it then we should do 3. Maybe we can add a boolean flag shouldRedirectIfInaccessible that defaults to false or some other way to indicate that the inaccessible report should not be ignored (assuming this is what happens now).

[marcaaron]

I think we've got enough of an understanding to hash this out in a PR.

[kadiealexander]

@rdjuric hey man! I can't seem to find you on Upwork to hire you for this issue. Could you please let me know if you've submitted this under a certain name?

[rdjuric]

Sorry @kadiealexander! Totally forgot to submit a proposal on Upwork. Just did it under the same name as my github.

[kadiealexander]

PR was merged 8 days ago, payment issued in Upwork today.