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

[Enhancement]: Graceful offline support #590 #591

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bardbess
Copy link

@bardbess bardbess commented Sep 5, 2023

When disconnected from the internet and Obsidian git still attempts to pull changes it throws some panicky error messages as mentioned in #590.

Check the connectivity to githubstatus before attempting to pull/push changes.

@rcv4
Copy link

rcv4 commented Sep 6, 2023

There is a another way to implement offline-support without the dependency on github. (Not every git repo is hosted on github)

Look up the navigator.onLine function given by typescript in the docs or used by another obsidian plugin here

i think that approach is more direct and better without the dependence on a 3rd party website

@bardbess
Copy link
Author

bardbess commented Sep 7, 2023

Thanks @rcv4 I didn't know about that, would have made things much easier.

Updated the PR to use navigator.onLine.

It will now also do a pull when the online state changes and had previously been set to offlineMode.

@bardbess
Copy link
Author

bardbess commented Oct 3, 2023

A little more detail on this PR if the moderators find it helpful.

From my understanding offlineMode is an existing flag that is set when repeated network attempts fail.

hasConnectivity will checks to see the device is online and If not set offlineMode saving simpleGit having to handle connectivity/network errors.

private onError(error: Error | null) {

When the device status changes to online AND an *event has set git-obsidian to offlineMode then reset offlineMode allowing remote attempts to retry, and also add a task to the queue.

*An event being repeated connectivity issues through the existing simpleGit offlineMode toggle or subsequent remote attempts that touch hasConnectivity.

@Vinzent03
Copy link
Collaborator

Sorry for taking so long to react to this pr. Where do you see these error messages? As notices in the obsidian ui or only as logs in the console? Using navigator.onLine doesn't really solve any issue, because even if it's true there still may be no internet connection.

@rcv4
Copy link

rcv4 commented Oct 8, 2023

Where do you see these error messages?
The error messages that are annoying appear in the UI

You dont necessarily need internet to connect to, for example, a locally hosted git server.

@bardbess
Copy link
Author

bardbess commented Oct 8, 2023

Sorry for taking so long to react to this pr. Where do you see these error messages? As notices in the obsidian ui or only as logs in the console? Using navigator.onLine doesn't really solve any issue, because even if it's true there still may be no internet connection.

Easiest way to reproduce it would be to set obsidian to pull on startup, turn your device wifi/data off and start the application. The message appears in the UI as a notification on both desktop and mobile. If it tries to push new changes every X period it gets annoying pretty quick.

image

The original offlineMode error handling remains unaffected and perhaps that will suffice for when there is a network connection but the server can't be reached.

@Vinzent03
Copy link
Collaborator

Vinzent03 commented Oct 8, 2023

I think the correct way to solve this is to implement the same offline detection of SimpleGit in IsomorphicGit by reading the error message. So no need for Navigator.onLine or sending additional http requests.

@bardbess
Copy link
Author

bardbess commented Oct 8, 2023

I think the correct way to solve this is to implement the same offline detection of SimpleGit in IsomorphicGit by reading the error message. So no need for Navigator.onLine or sending additional http requests.

navigator.onLine indicates wither on not your are able to connect to a local area network (LAN) or a router. If it returns false you don't have any network connectivity, local or otherwise.

SimpleGit currently attempts to send requests and after failed attempts sets the offlineMode flag to stop sending requests. I don't understand why you would send any requests if navigator.onLine returns false and your device has no network connectivity at all.

This change does not prevent catching and bubbling errors in IsomorphicGit and would complement the change you are suggestion not be superseded by it.

Here is a screenshot of Obsidian desktop with the same error (changing IsomorphicGit wont fix this).

obsidian

I have been using this build locally since raising the PR without any issues. The message after this PR on app boot.
obsidian-pr

@Vinzent03
Copy link
Collaborator

This change does not prevent catching and bubbling errors in IsomorphicGit and would complement the change you are suggestion not be superseded by it.

I don't know what you mean with this. Could you explain it further?

I still wouldn't depend on onLine, because it might prevent requests that don't exceed, but the same issue can still occur if onLine is true. Instead, the offline error detection has to be added to IsomorphicGit and the one in SimpleGit improved as it didn't catch your error message. Sending requests that fail, but are properly caught, don't hurt in any way.

@bardbess
Copy link
Author

bardbess commented Oct 9, 2023

Hi @Vinzent03

This PR observes the network connection changing. This compliments the error handling of SimpleGit. eg:

  1. The device is connected to "network A" (perhaps a local only network), Navigator.onLine is therefore true
  2. A request is made to the github repo. This request fails, and SimpleGit sets offlineMode to true.
  3. The user switches to a new network ("network B") but offlineMode is never reset.

This PR recognizes the switch in the network, resetting offlineMode and instantly attempts a new pull request.

Another example might be travelling (train/bus/subway/etc) in areas with poor or intermittent network reception or when switching your device from flight mode.

navigator.onLine detects the network connectivity of the device, local or otherwise. So it would only prevent the same device pulling/pushing from itself when that device had no connectivity. Do you have an example where navigator.onLine might be problematic?

@GollyTicker
Copy link
Contributor

navigator.onLine detects the network connectivity of the device, local or otherwise. So it would only prevent the same device pulling/pushing from itself when that device had no connectivity. Do you have an example where navigator.onLine might be problematic?

I don't know the exact behavior of navigator.onLine, but according to your definition, it'll definitively cause confusion, when the git-server is hosted in the local network or any other place which is still accessible despite github or obsidian.md not being accessible. (Which is the case in my setup.)

I like the general idea behind this feature, but would like to see one of the following alternatives:

  • only offer offline detection for specific public git hosts such as GitLab, GitHub, BitBucket, etc. If the git-url doesn't contain these, then don't change the behavior. This howeer is a bit unfair towards the people having their data not on these servers.
  • use git fetch with --dry to test connectivity. This is the best test ultimately and delegates properly to git. The response will also enable to distinguish between unreachability, connection refusal, invalid credencital/public-key. I prefer this, as it seems to be the cleanest option. However, one would need to decide, how often to test this connectivity and allow it to be configureable as well.
    What do the others think?

@bardbess
Copy link
Author

Hi @GollyTicker

I'm afraid you misunderstood me. By saying "navigator.onLine detects the network connectivity of the device, local or otherwise", I imply that if your device is connected to any network (regardless of internet access) then navigator.onLine will return true. If however you've device is not connected to any network (airplane mode, no signal, wifi-off) it returns false.

The example I gave above - which seems absurd to me - is if you have a single PC with no network devices/configuration to anything on which you have both your git server and in some other directory an obsidian vault with a cloned git repo - as in the same device has 2 copies of your git repo - and you are pushing and pulling from the 2nd one - not sure how you could even achieve this with no network configured - an obscure and potentially impossible setup which the existing offlineMode would probably block already.

From the docs

if the browser is not able to connect to a local area network (LAN) or a router, it is offline; all other conditions return true.

@bardbess
Copy link
Author

There is a another way to implement offline-support without the dependency on github. (Not every git repo is hosted on github)

Look up the navigator.onLine function given by typescript in the docs or used by another obsidian plugin here

i think that approach is more direct and better without the dependence on a 3rd party website

@rcv4 ,why is life so hard. 😮‍💨

@GollyTicker
Copy link
Contributor

@bardbess

Thanks for clarifying the behaviour of navigator.online. However, what about the dry git fetch as an alternative? After all, if could be used to also give more specific feedback to the user - as many git-newcomers don't know what to do, when they for instance see an denied public key.

@bardbess
Copy link
Author

Hi @GollyTicker

SimpleGit already has networkFailure error handling for this purpose, and as @Vinzent03 mentioned it would be good to implement something similar in IsomorphicGit, but why would you even attempt to fetch if your device is on airplane mode? And without leveraging the network detection handlers (used by navigator.onLine and online events) how can you trigger a pull/push once you turn airplane mode off.

@GollyTicker
Copy link
Contributor

Ok, I see. Using dry fetches is a complement rather than a replacement.

I don't know too many details on the rest, so I can't comment on that.

@bardbess
Copy link
Author

@GollyTicker you got it!

This PR has 2 commits:

  • The first commit just uses navigator.onLine to stop a request when there is no network connection e3a0be7
  • The second fires an event to queue an update when the network connects ce07cf4

Not sure if it helps when considering each change in isolation.

Bardoe Besselaar added 2 commits January 26, 2024 14:50
Check app online connectivity before attempting to pull/push changes
+ Set offline mode when there is no network connection and an remote attempt is made.
+ Reset offline mode flag when connectivity re-establishes and pull changes (delayed pull/push request)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants