feat: reload failed IPFS tabs when API becomes available #1092

whizzzkid · 2022-08-24T06:30:31Z

Closes #1010

In this PR:

I added support for reloading failed tabs when API is available again, by:
- By statically checking for failed tabs which belongs to local gateway instead of tracking all URLs.
- This reduces the complexity for tracking failed websites in content_scripts.
- More details on the implementation can be found in the relevant reloader.
I am also introducing a plugin mechanism that can register different reloader classes, this provides decent segregation of validation logic and also allows us to have reusable code for reloading.
Added relevant unit tests for 100% coverage.
Also updated the yarn.lock file and added a matching .editorconfig to configure IDEs.

Screencast:

output.mp4

whizzzkid

Adding explainer comments

whizzzkid · 2022-08-24T06:31:52Z

.editorconfig

+insert_final_newline = true
+charset = utf-8
+indent_style = space
+indent_size = 2


just to remove ambiguity from IDEs

whizzzkid · 2022-08-24T06:32:51Z

add-on/src/lib/ipfs-client/reloaders/localGatewayReloader.js

+    //   those pages.
+    // - The benefit we get from this approach is the static nature of just observing the tabs in their current state.
+    //   which reduces the overhead of injecting content scripts to track urls that were loaded after the connection
+    //   was offline, it may also need extra permissions to inject code on error pages.


this logic validates offline pages belonging to IPFS node.

whizzzkid · 2022-08-24T06:33:39Z

package.json

@@ -141,7 +141,7 @@
    "webextension-polyfill": "0.7.0"
  },
  "engines": {
-    "node": ">=14.15.0",
+    "node": ">=14.15.0 <17",


for some reason latest LTS (18) fails to build this. I did not investigate, but added check to prevent this.

+1. a lot of our packages and their dependencies are out of date, and there have been a lot of changes made w.r.t. esm support, so we've been in a tough spot. However, you can see our support goals at https://github.com/ipfs/ipfs-gui/tree/SgtPooki-version-support-definition#nodejs (We can link to main branch when ipfs/ipfs-gui#97 gets merged)

add-on/src/lib/ipfs-client/index.js

add-on/src/lib/ipfs-client/reloaders/index.js

add-on/src/lib/ipfs-client/reloaders/localGatewayReloader.js

SgtPooki · 2022-08-29T18:34:23Z

package.json

@@ -141,7 +141,7 @@
    "webextension-polyfill": "0.7.0"
  },
  "engines": {
-    "node": ">=14.15.0",
+    "node": ">=14.15.0 <17",


+1. a lot of our packages and their dependencies are out of date, and there have been a lot of changes made w.r.t. esm support, so we've been in a tough spot. However, you can see our support goals at https://github.com/ipfs/ipfs-gui/tree/SgtPooki-version-support-definition#nodejs (We can link to main branch when ipfs/ipfs-gui#97 gets merged)

whizzzkid · 2022-09-01T05:43:13Z

@SgtPooki looks like the builds are failing on windows-latest I created a PR to address that: #1094 can you please try that?

lidel

Thank you @whizzzkid, before we land this, do you mind

rebasing this to include fix from fix(ci): windows build #1094 (which already landed in main branch)
improving URL check to also work for subdomain gateways (details inline)
(optional) perhaps we could avoid title matching by leveraging onErrorOccurred? (details inline – not a blocker, could be descoped and filled as an issue for future improvement, lmk if you prefer that)

lidel · 2022-09-08T17:43:13Z

add-on/src/lib/ipfs-client/reloaders/localGatewayReloader.js

+     *   was offline, it may also need extra permissions to inject code on error pages.
+     */
+    return url.startsWith(this.customGatewayUrl) &&
+      (url.includes(title) || title.toLowerCase() === 'problem loading page')


two nits:

startsWith does not cover subdomain gateways

mind updating this check to use isIPFS.url && isIPFS.subdomain? (see https://www.npmjs.com/package/is-ipfs)

browser vendors may change the title (in Firefox it will be different for non-english locale)

(loose idea, not sure how feasible) potential improvement would be to leverage onErrorOccurred from src/lib/ipfs-request.js and inside of it, add isRecoverableViaOnlineApi which would save failed requests to local gateway in a cache similar to errorInFlight. Then, you could check if url is present in that cache, removing the need for matching titles. More complicated, but future-proof :)

Thanks, @lidel

the check should be isIPFS.url || isIPFS.subdomain and not isIPFS.url && isIPFS.subdomain added that.

I'll create a separate issue for that, It was on my mind but this PR is large as it is, I'll work on that issue separately.

@whizzzkid mind linking to the created issue or creating it and commenting here?

Here, #1097

Can you please assign this to me @SgtPooki/@lidel

lidel · 2022-09-08T17:46:26Z

add-on/src/lib/ipfs-client/reloaders/webUiReloader.js

+  validation ({ url }) {
+    const bundled = !url.startsWith('http') && url.includes('/webui/index.html#/')
+    const ipns = url.includes('/webui.ipfs.io/#/')
+    return bundled || ipns
+  }


nit: built-in webui at http://127.0.0.1:5001/webui/ returns redirect to http://127.0.0.1:5001/ipfs/{hardcoded-cid} (hardcoded-cid here)

It seems to be missing here, is it reloaded by some other means? (if not, we may want to add it)

This functionality was brought in from: https://github.com/ipfs/ipfs-companion/blob/main/add-on/src/lib/ipfs-client/index.js#L64-L68

But now that I think more about it, this will be handled in the localGatewayReloader check as isIPFS.url || isIPFS.subdomain returns true for http://127.0.0.1:5001/ipfs/bafybeibozpulxtpv5nhfa2ue3dcjx23ndh3gwr5vwllk7ptoyfwnfjjr4q hardcoded cid.

* main: fix(ci): windows build (ipfs#1094)

whizzzkid · 2022-09-11T22:04:27Z

@lidel @SgtPooki this is ready for re-review:

rebasing this to include fix from fix(ci): windows build #1094 (which already landed in main branch)
improving URL check to also work for subdomain gateways (details inline)
(optional) perhaps we could avoid title matching by leveraging onErrorOccurred? (details inline not a blocker, could be descoped and filled as an issue for future improvement, lmk if you prefer that): Track URLs that failed to load because gateway was down. #1097

Build is now 🟢 https://github.com/whizzzkid/ipfs-companion/actions/runs/3033417199

❤️

BigLep · 2022-09-19T12:48:23Z

@SgtPooki : is there any other feedback, or can this be merged?

SgtPooki · 2022-09-19T17:03:18Z

GUI triage: Lidel is going to give a look at this. We need to fix companion build pipeline before merging this

whizzzkid · 2022-09-19T17:39:39Z

@SgtPooki are the build pipelines broken again? I fixed the companion pipeline in #1094

SgtPooki · 2022-09-23T23:57:06Z

add-on/src/lib/ipfs-client/reloaders/localGatewayReloader.js

+     *   was offline, it may also need extra permissions to inject code on error pages.
+     */
+    return url.startsWith(this.customGatewayUrl) &&
+      (url.includes(title) || title.toLowerCase() === 'problem loading page')


@whizzzkid mind linking to the created issue or creating it and commenting here?

SgtPooki · 2022-09-24T00:00:39Z

@SgtPooki are the build pipelines broken again? I fixed the companion pipeline in #1094

Sorry we missed that =) Gave my approval but I've got very little experience in this repo so I'm leaving the PR open for @lidel to handle when he gets to it. I'd ping him Monday if it's not merged by then

SgtPooki · 2022-09-26T16:44:07Z

GUI triage: Lidel is going to give a look at this. We need to fix companion build pipeline before merging this

lidel

Thank you @whizzzkid and @SgtPooki

LGTM, there are unrelated issues caused by old dependencies (will fill separate issues if I have no time to fix them myself), but the functionality of this PR is good!

I'm merging it to unblock follow-up work from #1097

lidel · 2022-09-26T13:20:10Z

test/functional/lib/ipfs-client/reloaders/reloaders.test.js

+    it('should handle internal tab reloading', function () {
+      const tabs = [{
+        url: 'http://127.0.0.1:8080/ipfs/cid/wiki1',
+        id: 1
+      }, {


@whizzzkid in browser context we also need to test subdomain gateway at localhost (one for .ipns.localhost:8080 and one for .ipfs.localhost:8080) That is what is used by users by default these days.

Not blocking this PR on this, but added it as additional task to #1097 :)

whizzzkid added 12 commits August 22, 2022 03:08

Modifying how to check for bad state

5275c15

hooking up to state change

2f68cbd

Improving logic

75579fc

Adding engine check as build fails on 17/18

0e0cfd5

Segregating Reloader Logic

1714406

Fixing comments

c334d06

Running Lint

4e6a86c

yarn clean

1686606

Adding more cases for offline detection

3c6c2af

Fixing messages and unnecessary constructor

7f6287d

Adding unit tests

0fde4f9

Adding editor config for IDEs

2f37bab

whizzzkid requested a review from lidel as a code owner August 24, 2022 06:30

This comment was marked as resolved.

Sign in to view

whizzzkid changed the title ~~Modifying how to check for bad state~~ [Issue-1010] Reload failed IPFS tabs when API becomes available Aug 24, 2022

whizzzkid commented Aug 24, 2022

View reviewed changes

SgtPooki requested changes Aug 29, 2022

View reviewed changes

whizzzkid added 3 commits August 29, 2022 22:34

Fixing reloaders jsdoc

e437eec

Fixing comments

0652353

Reverting yarn.lock to fix ci.

6faff55

whizzzkid requested review from SgtPooki and removed request for lidel August 30, 2022 05:08

Fixing lint

a645a85

lidel self-requested a review September 1, 2022 15:03

lidel changed the title ~~[Issue-1010] Reload failed IPFS tabs when API becomes available~~ fix: reload failed IPFS tabs when API becomes available Sep 8, 2022

lidel changed the title ~~fix: reload failed IPFS tabs when API becomes available~~ feat: reload failed IPFS tabs when API becomes available Sep 8, 2022

lidel requested changes Sep 8, 2022

View reviewed changes

whizzzkid added 2 commits September 11, 2022 14:44

Merge branch 'main' into feature/1010-reload-failed-IPFS-tabs

b69a468

* main: fix(ci): windows build (ipfs#1094)

Adding check using is-ipfs

b420dc7

Adding yarn.lock since build failed elsewhere

404a6ff

whizzzkid mentioned this pull request Sep 11, 2022

Track URLs that failed to load because gateway was down. #1097

Open

1 task

whizzzkid requested review from lidel and SgtPooki and removed request for SgtPooki and lidel September 11, 2022 22:05

SgtPooki requested a review from lidel September 19, 2022 17:03

SgtPooki approved these changes Sep 23, 2022

View reviewed changes

SgtPooki assigned lidel and SgtPooki Sep 24, 2022

SgtPooki added status/ready Ready to be worked need/maintainer-input Needs input from the current maintainer(s) labels Sep 24, 2022

lidel approved these changes Sep 26, 2022

View reviewed changes

lidel merged commit 8a33b6c into ipfs:main Sep 26, 2022

whizzzkid deleted the feature/1010-reload-failed-IPFS-tabs branch September 27, 2022 06:23

github-actions bot mentioned this pull request Feb 16, 2023

chore(main): release ipfs-companion 2.23.0 #1159

Closed

github-actions bot mentioned this pull request Sep 21, 2023

chore(main): release 3.0.1 #1288

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: reload failed IPFS tabs when API becomes available #1092

feat: reload failed IPFS tabs when API becomes available #1092

whizzzkid commented Aug 24, 2022 •

edited by lidel

Loading

This comment was marked as resolved.

whizzzkid left a comment

whizzzkid Aug 24, 2022

whizzzkid Aug 24, 2022

whizzzkid Aug 24, 2022

SgtPooki Aug 29, 2022

SgtPooki Aug 29, 2022

whizzzkid commented Sep 1, 2022

lidel left a comment •

edited

Loading

lidel Sep 8, 2022

whizzzkid Sep 11, 2022

SgtPooki Sep 23, 2022

whizzzkid Sep 24, 2022

lidel Sep 8, 2022 •

edited

Loading

whizzzkid Sep 11, 2022

whizzzkid commented Sep 11, 2022 •

edited

Loading

BigLep commented Sep 19, 2022

SgtPooki commented Sep 19, 2022

whizzzkid commented Sep 19, 2022

SgtPooki Sep 23, 2022

SgtPooki commented Sep 24, 2022 •

edited

Loading

SgtPooki commented Sep 26, 2022

lidel left a comment

lidel Sep 26, 2022

feat: reload failed IPFS tabs when API becomes available #1092

feat: reload failed IPFS tabs when API becomes available #1092

Conversation

whizzzkid commented Aug 24, 2022 • edited by lidel Loading

This comment was marked as resolved.

whizzzkid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whizzzkid commented Sep 1, 2022

lidel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidel Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whizzzkid commented Sep 11, 2022 • edited Loading

BigLep commented Sep 19, 2022

SgtPooki commented Sep 19, 2022

whizzzkid commented Sep 19, 2022

Choose a reason for hiding this comment

SgtPooki commented Sep 24, 2022 • edited Loading

SgtPooki commented Sep 26, 2022

lidel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whizzzkid commented Aug 24, 2022 •

edited by lidel

Loading

lidel left a comment •

edited

Loading

lidel Sep 8, 2022 •

edited

Loading

whizzzkid commented Sep 11, 2022 •

edited

Loading

SgtPooki commented Sep 24, 2022 •

edited

Loading