Skip to content
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

Request the native app to reload the direct editing view on 403 errors #5662

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nicofrand
Copy link

📝 Summary

When it receives a 403 error it triggers a reload event, catchable by the android app.

Warning

I don't know how this app works/should behave in some cases, and this PR needs discussion.

  1. Right now it keeps retrying the requests even though the 403 error is detected. Can I just stop the retries (they will fail anyway)? What about Direct edition outside the android app (which will not catch the reload event)?
  2. The comment says "the session is invalid or the document is read only". Can I make sure the session is invalid (I should not reload the document in case it is read only I guess?)

@max-nextcloud
Copy link
Collaborator

Hey @nicofrand

Thanks a lot for your PR.

There are different scenarios which may cause a 403:

  • Session expired or was cleaned up. In this case we would like a reload as that will cause text to reconnect and pick up the new session.
  • User lost access to the document permission wise. In this case there's no point in retrying but a reload will actually show the permission error i think.
  • The document is read only to begin with. This is a bit of an awkward situation right now. The user is not allowed to change the document but we would like them to be able to follow changes as they happen. We use yjs and it expects to see status updates at least from the user themself every 15 seconds. However we block such updates on the server as the user may not change the document. We should probably allow awareness changes and just block all other types of messages.
  • Right now it keeps retrying the requests even though the 403 error is detected. Can I just stop the retries (they will fail anyway)?

In the case of an expired session reconnecting should fix the connection. But that's not what's happening right now i guess. Will need to investigate.

What about Direct edition outside the android app (which will not catch the reload event)?

I think displaying a message that asks for reloading the page would be a good start. We already do that in some cases.

  • The comment says "the session is invalid or the document is read only". Can I make sure the session is invalid (I should not reload the document in case it is read only I guess?)

You should be able to check $editor.isEditable to rule out read only documents causing this.

@max-nextcloud max-nextcloud self-assigned this Apr 23, 2024
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

@nicofrand
Copy link
Author

nicofrand commented Apr 30, 2024

Hi!

Thanks for your comments.

You should be able to check $editor.isEditable to rule out read only documents causing this.

OK but the 403 occurs in the SyncService, which does not have access to $editor as far I can tell. It can just emit things.
So it can't just return before throw new Error('Failed to apply steps. Retry!', { cause: err })?

I think displaying a message that asks for reloading the page would be a good start. We already do that in some cases.

Could you explain how to test a DirectEdition outside the android app? I was not able to find a way…
I will display a message instead of callMobileMessage('reload') but is there an existing/proper way to do so already? Or do I need to manipulate the DOM myself?
And how do I know I am not in an android app?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

Text editor loses connection when app sent to background then Reconnect fails
3 participants