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

feat: recovery page when local gateway is unreachable #1125

Merged
merged 42 commits into from
Feb 3, 2023
Merged

Conversation

whizzzkid
Copy link
Contributor

@whizzzkid whizzzkid commented Dec 23, 2022

Addresses: #1090

In this PR:

  • Recovery mechanism for URLs that are not reachable because the node is offline.
  • Adds a button to redirect to public url.
  • Refactor
  • Recovery Page UX

Screenshot:

Recovery Process:

recovery_companion_2x.mp4

Auto-Reload:

recovery_auto-reload.mp4

Copy link
Contributor Author

@whizzzkid whizzzkid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explainers.

@@ -33,7 +33,10 @@
"icons/png/ipfs-logo-off_38.png",
"icons/png/ipfs-logo-off_128.png",
"icons/ipfs-logo-on.svg",
"icons/ipfs-logo-off.svg"
"icons/ipfs-logo-off.svg",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure how this is persisted on CI, locally this gets overwritten.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@whizzzkid This file is a template used for generating final add-on/manifest.json for specific runtime: firefox, chromium, or brave (production brave used chromium, but we need this for local testing).

Generation happens in ci/update-manifest.sh (triggered during build with some env vars). It was way more complex in the past when we had beta channels, we could simplify build pipeline these days, but did not bother since it works fine.

Comment on lines 148 to 151
if (!state.connected && request.type === 'main_frame' && sameGateway(request.url, state.gwURL)) {
const publicUri = ipfsPathValidator.resolveToPublicUrl(request.url, state.pubGwURLString)
return { redirectUrl: `${dropSlash(runtimeRoot)}${recoveryPagePath}#${publicUri}` }
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

once we know we're not connected we can load the recovery page and append the public URI as a hash

Comment on lines 45 to 52
Object.defineProperty(state, 'connected', {
// TODO: make quick fetch to confirm it works?
get: function () { return this.peerCount > offlinePeerCount + 1 }
})
Object.defineProperty(state, 'nodeActive', {
// TODO: make quick fetch to confirm it works?
get: function () { return this.peerCount !== offlinePeerCount }
})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented the missing TODOs, will move this to a proper class later.

@whizzzkid whizzzkid marked this pull request as ready for review December 29, 2022 11:26
@whizzzkid
Copy link
Contributor Author

@juliaxbow implemented v0 design, I think it's good enough for now, we can tweak the design more after we have more clarity in #1090

node-offline

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @whizzzkid, this is a nice quality of life improvement.

Before we merge:

  • add regression tests
    • I think a simple modifyRequest.onBeforeRequest test in test/functional/lib/ipfs-request-gateway-redirect.test.js should suffice – we want somehting to guard the behavior so it does not break during future refactors.
    • Feel free to add E2E that tests behavior of recovery page if you have time.
  • address minor bugs / cosmetic changes to the presentation suggested below
  • refine design to follow suggestion / mockup from Improve UX when localhost gateway is offline #1090 (comment)

add-on/src/lib/ipfs-request.js Outdated Show resolved Hide resolved
add-on/src/lib/helpers.js Outdated Show resolved Hide resolved
${logo({ path: logoPath, size: logoSize, isIpfsOnline: isIpfsOnline })}
<p class="montserrat mt3 mb0 f2">${i18n.getMessage('page_landingWelcome_logo_title')}</p>
${renderLogo(isIpfsOnline)}
${showTitle ? `<p class="montserrat mt3 mb0 f2">${i18n.getMessage('page_landingWelcome_logo_title')}</p>` : ''}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTML is not rendered properly:

2023-01-11-160547_1131x611_scrot

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking, fixed!

add-on/src/recovery/recovery.html Show resolved Hide resolved
add-on/_locales/en/messages.json Outdated Show resolved Hide resolved
add-on/src/recovery/recovery.js Outdated Show resolved Hide resolved
add-on/_locales/en/messages.json Outdated Show resolved Hide resolved
add-on/src/recovery/recovery.js Outdated Show resolved Hide resolved
Comment on lines 45 to 48
Object.defineProperty(state, 'connected', {
// TODO: make quick fetch to confirm it works?
get: function () { return this.peerCount > offlinePeerCount + 1 }
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This + 1 will cause bugs when your local node is running, but your internet connection goes down. Imagine you opened something, it got cached on your IPFS node, then you shut down your laptop, go to a place without iternet and you opened laptop and restarted browser and there is a tab with localhost gateway URL.

User should be able to browse IPFS resources cached on their IPFS node when WAN is down, but due to peerCount > 0 requirement here Companion will show recovery page, forbidding them from leveraging IPFS cache.

Mind removing this and only triggering recovery when peerCount === offlinePeerCount?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right I did not think about that.

// TODO: make quick fetch to confirm it works?
get: function () { return this.peerCount > offlinePeerCount + 1 }
})
Object.defineProperty(state, 'nodeActive', {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know if things defined via Object.defineProperty will survive postMessage serialization present in places where we use browser.runtime.connect ?

I vaguely remember that we did not do getters like this in the past because it did not survive serialization (maybe browser fixed it these days?). That is why we read peerCount field directly (without getters) and compare it to offlinePeerCount const.

By adding these getters here, we are having two ways of doing online check in the codebase, which over time leads to errors.

My suggestion is to remove these Object.defineProperty to keep this PR scope small, and if you want to add them, do that in separate PR that also refactors all places which act on peerCount (if possible, but I feel it may not be worth the work).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it does: https://github.com/ipfs/ipfs-companion/pull/1125/files#diff-cf4a768af4d2baab2a1c0a5862f50af6239b155754f4a9ea90b43e465f115d88R148

I need this to validate if we need to recover the client. However I didn't want to touch too much of this, I'm just trying to follow the adjacent code pattern. I created #1129 to tackle this again in the future.

@whizzzkid
Copy link
Contributor Author

@lidel addressed your concerns, here is the new UI, cc: @juliaxbow

@whizzzkid whizzzkid requested a review from lidel January 13, 2023 07:14
app.use(createWelcomePageStore(i18n, runtime))
// Register our single route
app.route('*', (state) => {
browser.runtime.sendMessage({ telemetry: { trackView: 'recovery' } })
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SgtPooki added view for recovery pages.

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@whizzzkid thanks!
Code lgtm, but before we merge, the layout needs a few final touches to match aesthetic proposed by Julia:

Current state Original mockup
image

We've added some more text, so I propose balancing things by moving some content to the left side:

212259625-24484f1e-0cdd-4e8a-ab4b-15058652ce94

  • left side: remove companion cube, name and version, and instead show ⚠️ icon and the first header there
    • Note we already have nice orange icon for this on the welcome screen, when node is offline, just reuse it:

      2023-01-31-230150_375x183_scrot

  • remove topbar (it is redundant if we have left panel)
  • fix css of links: they should not have underline, and should not be purple when :visited
  • fix css of the green button (round radius, padding, color

@whizzzkid
Copy link
Contributor Author

whizzzkid commented Feb 3, 2023

Screenshot 2023-02-02 at 11 38 50 PM

@lidel

@whizzzkid whizzzkid requested a review from lidel February 3, 2023 07:09
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @whizzzkid, we've ended up with a really user-friendly interface! ❤️

Some notes:

  • I meant Improve UX when localhost gateway is offline #1090 (comment) because the top bar is only present in Options page. Welcome screen, which has the split design, does not have it and we should keep things minimalistic.
  • I've shortened info text and applied same link style as on Welcome page.
  • Let's ship it. Merging.
  • (thoughts on follow-up work) We can refine user experience further by showing a similar screen when a remote HTTP server fails, but it is a domain name with DNSLink, or the URL that is a public gateway. Currently, we just silently retry via local gateway, but showing intermediate page like this would be a better UX (removes surprising behavior and bugs, allows us to educate user about the value of IPFS around making websites more robust and immune to DoS or censorship).

add-on/_locales/en/messages.json Outdated Show resolved Hide resolved
add-on/_locales/en/messages.json Outdated Show resolved Hide resolved
add-on/_locales/en/messages.json Outdated Show resolved Hide resolved
add-on/_locales/en/messages.json Outdated Show resolved Hide resolved
add-on/src/recovery/recovery.js Outdated Show resolved Hide resolved
add-on/src/recovery/recovery.js Outdated Show resolved Hide resolved
add-on/src/recovery/recovery.js Outdated Show resolved Hide resolved
add-on/_locales/en/messages.json Outdated Show resolved Hide resolved
@lidel lidel merged commit a74fbb3 into main Feb 3, 2023
@lidel lidel deleted the feat/recover-client branch February 3, 2023 13:52
@lidel lidel changed the title feat: recover client if localhost is unreachable feat: recovery page when local gateway is unreachable Feb 3, 2023
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.

2 participants