feat: improved UX when opening IPFS URIs #1966
Conversation
hacdias
force-pushed
the
feat/ux-protocol-handler
branch
from
061e79d to
b319b8c
Compare
There was a problem hiding this comment.
Thank you, this is long time due!
Small asks
- replace "URI" with "address" (diffs inline)
- add IPFS cube logo to the Ask window (centered, above "How would you like.." sentence)
Note: the Explore section does not support IPNS. Thus, I decided to open on the Files page if the user opens an IPNS URL and selects the Explore section by default.
The other option is to ask separately for IPNS and IPFS. That is possible but will duplicate the options in the settings.
Hm.. we don't want to duplicate settings, and opening Files instead of Explore looks like a bug.
I think we should fix ipfs/ipfs-webui#1884 before merging this PR, so there are no surprises around the way IPFS addresses are handled.
.. or just hide "Explore" option for now, so we don't need to wait for ipfs/ipfs-webui#1884
src/protocol-handlers.js
Outdated
| } | ||
|
|
||
| const action = await getAction() | ||
| let base = 'https://dweb.link' |
There was a problem hiding this comment.
💭 hardcoding this may be confusing because ipfs-webui now allows user to customize the public gateway (added in ipfs/ipfs-webui#1834):
I think Electron should be able to reach out to the renderer and see if custom public gateway was set in window.localStorage under ipfsPublicGateway (+ JSON unwrapping due to local-storage.js weirdness).
That way we could respect user choice (and if not present, default to dweb.link
There was a problem hiding this comment.
This is a great idea, but I will also need to work on a hook to make sure this value gets updated on IPFS Desktop every time it is updated on IPFS Web UI.
There was a problem hiding this comment.
@hacdias I was thinking about simple, low-effort implementation that respects user choice made in ipfs-webui Settings:
- every time you need to generate shareable link in ipfs-desktop, read the value from localStorage in ipfs-webui in renderer process.
- not need for copy of the value does not be stored/updated in ipfs-desktop
There was a problem hiding this comment.
@lidel that is reasonably simple if we wait for the Web UI window to be ready. We have to wait for it to be ready to have that working. So if you open an IPFS link, IPFS Desktop has to boot up and wait for the Web UI window to be ready until the link is actually open. That could feel quite slow. That's why I thought about keeping a copy on Desktop's settings such that we do not need to wait.
There was a problem hiding this comment.
Then, should we do the same for the "Download CID" window? We're using the same template after all.
I'll do that. |
There was a problem hiding this comment.
Polishing the UX
I took it for a spin via npm start -- ipns://en.wikipedia-on-ipfs.org to trigger the one-time choice dialog, current options are:
My initial impressions:
- 🎨 small UX asks
- 🧊 needs IPFS cube as a visual hint to indicate where the question comes from
-
Then, should we do the same for the "Download CID" window? We're using the same template, after all.
@hacdias yes, sounds sensible and it would look good there too
-
- 🆔 could we show the UR Ithat triggered the dialog as part of the question? I'm thinking
How would you like to open IPFS addresses like ipns://en.wikipedia-on-ipfs.org?- makes it easier to connect the dots ("i clicked on this ipns:// thing and this happened")
- 🧊 needs IPFS cube as a visual hint to indicate where the question comes from
- 💭 I started questioning myself: is there a value in having separate browser options for public and local gateway?
- 💢 Beginner/casual users would not even know what this choice means
- 💡 this could be mitigated by adding "(safe default choice)" next to pre-selected "public gateway" option
- 💢 If user selected "public" but their browser has redirect to local (Brave, Companion) that looks like a bug, because they wanted public link, and ended up on local
- 💢 If user selected "local" but the node is down, then links either fail or we need to use public gateway anyway
- I've seen folks strugging with this in ipfs-companion – the bug reports go away only if we redirect to public gw when local is dead.
- 💡 My gut feeling is that we should only have the public gateway option (simple "In my web browser") – kicking the routing decision to the browser.
- 💚 Browsers with IPFS support (Brave or IPFS Companion) would redirect public gateway link to local gateway anyway
- 💚 regular browsers would load the original URL, there is no footgun when local node is offline
- 💢 Beginner/casual users would not even know what this choice means
Thoughts?
cc'ing @autonome @SgtPooki (not urgent, but before we ship this, would appreciate more 👀 on this, both from the perspective of a native speaker, and general UX)
src/protocol-handlers.js
Outdated
| } | ||
|
|
||
| const action = await getAction() | ||
| let base = 'https://dweb.link' |
There was a problem hiding this comment.
@hacdias I was thinking about simple, low-effort implementation that respects user choice made in ipfs-webui Settings:
- every time you need to generate shareable link in ipfs-desktop, read the value from localStorage in ipfs-webui in renderer process.
- not need for copy of the value does not be stored/updated in ipfs-desktop
Done.
This is easy to do, but we have to make sure it doesn't overflow or wrap to the next line, otherwise some window text will be hidden. The maximum size would also vary with translations so I'm not sure what's the best way to proceed here. I will defer the remaining updates once we have @autonome's and @SgtPooki's feedback regarding the gateway options. |
|
I agree 100% with all the callouts from lidel. A few additional comments:
I would reduce the number of options to only either: open in browser, open in ipfs desktop, and then let ipfs-companion/browser handle the rest, since they will override whichever decision is made in ipfs-desktop anyway. What do you think @lidel? And if we did do this, should the browser option always use local? If a user is running ipfs-desktop, then they should have a local node running at the time ipfs-desktop opens the link in the browser, right? |
Yep, this sounds like the best way to do this.
Hm.. yes. If we have a generic "Open in browser" then yes, this makes sense and provides better experience for users with browser that does not understand IPFS, because links will open instantly from local node, instead of YMMV provided by a public gateway. ipfs-desktop could make a quick test to confirm local gateway is still online, and use its address instead of public one. ipfs-webui is already doing that type of check (it checks if local gateway port is functional, it also confirms if OS supports subdomains on To be specific, if we want to open from local node by default, we need to:
|
hacdias
force-pushed
the
feat/ux-protocol-handler
branch
from
90b25e0 to
69e4619
Compare
|
I will work on those changes then. |
License: MIT Signed-off-by: Henrique Dias <[email protected]>
Co-authored-by: Marcin Rataj <[email protected]>
Co-authored-by: Marcin Rataj <[email protected]>
Co-authored-by: Marcin Rataj <[email protected]>
Co-authored-by: Marcin Rataj <[email protected]>
License: MIT Signed-off-by: Henrique Dias <[email protected]>
hacdias
force-pushed
the
feat/ux-protocol-handler
branch
from
69e4619 to
3b7806e
Compare
|
Just updated the PR to match the conversation above.
Disclaimer: I tested this on macOS. It seems that the |
There was a problem hiding this comment.
Thanks for the updates @hacdias . Many comments are questions because I'm newer to this repo, but the ctx refactor has helped me to gain a bit of understanding of the current problems we face with the ipfs-desktop, so I hope the non-question comments are helpful =)
src/tray.js
Outdated
| NAMESYS_PUBSUB_KEY, | ||
| ASK_OPENING_IPFS_URIS |
There was a problem hiding this comment.
@lidel @achingbrain This is why we need to use trailing commas. How can I go about updating the styling across our repos so that we can allow/require trailing commas in our code? These diffs don't need to be this noisy. git blaming is also a PITA when we try to find out who originally added NAMESYS_PUBSUB_KEY, or whatever the non-trailing-comma-having previous line ends up being in PRs.
|
@SgtPooki I think I addressed all of your comments! |
| const { status } = await fetch( | ||
| `${url}/ipfs/bafkqae2xmvwgg33nmuqhi3zajfiemuzahiwss` | ||
| ) | ||
| return status === 200 |
There was a problem hiding this comment.
do we need a status=200 or could we check for ok?
| url = new URL(url) | ||
| url.hostname = `bafkqae2xmvwgg33nmuqhi3zajfiemuzahiwss.ipfs.${url.hostname}` | ||
| const { status } = await fetch(url.toString()) | ||
| return status === 200 |
There was a problem hiding this comment.
would using ok be sufficient or do we require an explicit 200?
There was a problem hiding this comment.
Code borrowed from Web UI: https://github.com/ipfs/ipfs-webui/blob/4bf78a47fcb38872edbe55a5046ae3c7d3fe1a10/src/bundles/config.js#L113-L138
Not sure if we need explicit 200 or any OK suffices. /cc @lidel
| const checkIfSubdomainGatewayUrlIsAccessible = async (url) => { | ||
| try { | ||
| url = new URL(url) | ||
| url.hostname = `bafkqae2xmvwgg33nmuqhi3zajfiemuzahiwss.ipfs.${url.hostname}` |
There was a problem hiding this comment.
What is the hardcoded CID in bafkqae2xmvwgg33nmuqhi3zajfiemuzahiwss.ipfs.${url.hostname}?
Should that be saved in consts.js?
There was a problem hiding this comment.
Same as before: I did not write this code, but it seems that it is some safe CID (ipfs/ipfs-webui@ebda764) for "Welcome to IPFS :-)"
There was a problem hiding this comment.
This is a CID which has Welcome to IPFS :-) string inlined inside identity Multihash.
Main reason for using this exotic multihash type is to avoid hitting DHT/datastore – gateway can respond immediatelly after decoding CID.
| * @param {ParsedUrl} parsedUrl | ||
| * @returns | ||
| */ | ||
| async function getGatewayUrl (ctx, { protocol, hostname, path }) { |
There was a problem hiding this comment.
@lidel do we have an ipfs-utils or other library where we can put our higher level functions that are used across the stack? It could help prevent us from getting into a situation in the future where all of our libraries are using out of date low-level ipfs deps.
There was a problem hiding this comment.
Afaik no.
There are things like https://github.com/ipfs/js-ipfs-utils (legacy, does not seem to be maintained) and https://github.com/ipfs/js-ipfs/tree/master/packages/ipfs-core-utils/src (part of js-ipfs), but they pull-in too many dependencies.
|
@SgtPooki solved the conflicts. It seems that the CI is taking way too long downloading Web UI for the tests. We may have to restart them later. |
There was a problem hiding this comment.
@SgtPooki @hacdias I just realized this PR comes with an awful UX footgun 😿
If user tries to open ipns://en.wikipedia-on-ipfs.org/wiki/ and decides to do that in IPFS WebUI, it will fail because ipfs-webui is not optimized for showing huge HAMT-sharded UnixFS directories, and /ipns/en.wikipedia-on-ipfs.org/wiki/ is one (has ~20M items).
Due to this, I feel we need to mark this as blocked until we add a smart HAMT handling.
Upstream issues:
Co-authored-by: Marcin Rataj <[email protected]>
hacdias
force-pushed
the
feat/ux-protocol-handler
branch
from
9c8b2b9 to
270d60d
Compare
|
This ended up no longer feasible, see #1646 (comment) |
This fixes #1646 by adding a dialog when opening an IPFS or IPNS with IPFS Desktop:
The UI matches the other dialogs we use on IPFS Desktop, which try to resemble a native dialog the most it can.
The option to ask when opening is also added under the preferences in the menubar tool:
Note: the Explore section does not support IPNS. Thus, I decided to open on the Files page if the user opens an IPNS URL and selects the Explore section by default.
The other option is to ask separately for IPNS and IPFS. That is possible but will duplicate the options in the settings.