feat: import files to MFS #810

colinfruit · 2019-11-07T02:10:28Z

This PR changes the upload target from ipfs add to MFS. Currently, it uses the default directory ipfs-companion-uploads with the user option to change this on the upload page. On successful upload, it will open the Web UI, at the directory used for the upload.

closes #415 and closes #635

lidel

This is awesome, thank you @colinfruit!
Apologies for not getting to this sooner.

Big 👍, but before we merge this, some things to address:

Please change labels and variable names from "upload" to "import"
(eg. "import to IPFS", /ipfs-companion-imports/ directory on MFS, etc).
Context: We generally avoid the 'upload' word, because it suggests data is pushed to some external machine, but thats not how IPFS works, and ipfs add may be renamed to ipfs import in the future (see RFC: unite the files APIs ipfs-inactive/interface-js-ipfs-core#552).
Would it be possible to expose the name of default directory for uploads as an editable setting on Preferences screen? Its ok to add it to "Experiments" section for now, as we may change it in the future.
I am bit worried what happens to /ipfs-companion-imports/ when user uploads 10th, 100th file. To illustrate, I've shared multiple files over times, and today I shared death-button.jpg. After it got imported, Companion opened /ipfs-companion-imports/ and the file is lost in the noise.
(see screenshot)
Potential solution is to shard per timestamp (/ipfs-companion-imports/2019-11-11_184500) to keep this in check. Perhaps we could make the default /ipfs-companion-imports/%Y-%m-%d_%H%M%S/ and replace timestamp on the fly, enabling people to define their own destinations, eg. if they want to shard per day instead of per second?

lidel

Two more things:

add-on/src/popup/quick-upload.js

lidel · 2019-11-11T18:02:02Z

add-on/src/popup/quick-upload.js

    }
-    let result
+    state.progress = `Uploading ${streams.length} files...`


Note to self: it looks like we are losing import progress report, but it never worked correctly over HTTP API (https://github.com/ipfs/js-ipfs-http-client/issues/842), so its ok to drop it for now.

lidel · 2019-11-12T00:39:31Z

add-on/src/lib/ipfs-companion.js

+    get openWebUiAtDirectory () {
+      return openWebUiAtDirectory
+    },


Ugh, another realization: we can't open webui if embedded js-ipfs is used in non-Brave browser.
We need to check if getState().ipfsNodeType === 'embedded' and open resource at public gateway instead (Brave uses embedded:chromesockets, so it won't match and open WebUI)

Perhaps we could make this even more useful:
What if we add toggle "Open imported files in WebUI"?
Enabled by default, but when disabled it would open at Gateway (restoring old behavior).

Sounds good. It would be great to find a way of opening the WebUI when using embedded nodes in a subsequent issue. Has this been looked at before?

Embedded node is unable to open port to provide HTTP API, so the only option I see is to open WebUI from third party server, eg. https://webui.ipfs.io and make it use API of embedded node from Companion via window.ipfs. At the moment we don't support window.ipfs in WebUI because it is sandboxed, and WebUI requires full read-write access to MFS and config.
Details in: #514

colinfruit · 2019-11-15T00:09:41Z

Thanks, @lidel!

Please change labels and variable names from "upload" to "import"

Makes sense. I changed all the user-facing labels this PR introduces from "upload" to "import". I did not change the variable names, because it feels inconsistent with the existing code. References to upload are everywhere, even the file name is quick-upload.js. If you want to make an issue, I'll be glad to remove upload references throughout the code base.

Would it be possible to expose the name of default directory for uploads as an editable setting on Preferences screen?

Sure. Also, I removed it from the upload screen, because it seemed redundant. On the other hand, it provides useful functionality there as well. What do you think?
Edit: added it back.

I am bit worried what happens to /ipfs-companion-imports/ when user uploads 10th, 100th file.

I'm looking into the best way to handle user-generated date symbols (%Y-%m-%d), and the best way to demonstrate this functionality to the user. What if they removed them, and then forgot what the symbols were, or weren't sure what they meant to begin with? Maybe a result field on the Preferences screen (and upload screen if we return the functionality) that show what directory will be used? Or an instruction in the description?
Edit: added the feature.

lidel · 2019-11-19T00:22:37Z

Thank you @colinfruit! I am travelling this week, but will review this as soon I have some uninterrupted time.

lidel

@colinfruit Apologies for slowness on my end. Good news is that after 32h travel I am finally back :-)

This PR is great, we are nearly there: small cosmetic asks below and inline.

Let's add "File Import" section to Options page and move related settings there
(This can be a separate PR if you want to ship this sooner)
Please change newly added labels, variables and keys in this PR to use "import" instead of "upload" (mainly uploadDir → importDir). To keep this PR small I created Language: replace "upload" with "import" #817 for the rest.
- FYI I've added you as a Collaborator, so you no longer need to work from your fork, you can close this original repo and create feature branches in it (it makes easier to work on PRs together)

Can't wait for pushing this over the finish line ❤️

lidel · 2019-11-25T22:07:48Z

add-on/src/options/forms/experiments-form.js

+          <input
+            id="uploadDir"
+            type="text"
+            pattern="^\/(.*)"


Trailing slash may be missing. To avoid bugs caused by that, either require it to end with / (easy, fix below) or programmatically add it if missing (harder)

Suggested change

pattern="^\/(.*)"

pattern="^\/(.+)\/"

I added this before import happens in quick-upload.js, to account for the potential case of someone forgetting the trailing slash on the import page. It handles both cases, and prevents the user from having to guess that a trailing slash is necessary. https://github.com/ipfs-shipyard/ipfs-companion/pull/810/files#diff-9984375a8301f7832fa0da0a5aafd803R75

add-on/src/popup/quick-upload.js

add-on/_locales/en/messages.json

colinfruit · 2019-11-26T03:03:10Z

Let's add "File Import" section to Options page and move related settings there

Sounds good, one thing we need is to handle preloadAtPublicGateway more consistently.

Please change newly added labels, variables and keys in this PR to use "import" instead of "upload" (mainly uploadDir → importDir). To keep this PR small I created #817 for the rest.

Sure.

colinfruit · 2019-11-26T03:08:39Z

add-on/src/popup/quick-upload.js

+    // unless and embedded node is in use (no access to web UI)
+    // in which case, open resource.
+    if (state.ipfsNodeType === 'embedded' || !state.openViaWebUI) {
+      await ipfsCompanion.uploadResultHandler({ result, openRootInNewTab: true })


This is not working as expected for me tonight, when importing multiple files without using the webUI, the context menu simply hangs. Maybe it's some weirdness with my connection; I'll take a look at this tomorrow.

Same issue this morning, importing multiple files and opening them in a context menu hangs. However, it is reproducible on current master, so I won't deal with it in this PR.

I believe the issue you described is due to slow content discovery by the gateway at https://ipfs.io/ipfs/.
Embedded node in Firefox/Chrome can't run own gateway, so we use public one instead.

Potential fix would be to update preloadAtPublicGateway and apart from existing HTTP HEAD request to user-specified public gateway, do additional recursive preload via HTTP GET request to one of preload nodes:

https://node0.preload.ipfs.io/api/v0/refs?arg={CID}&r=true https://node1.preload.ipfs.io/api/v0/refs?arg={CID}&r=true

In case of "quick-upload" screen, the CID should be of the wrapping directory.

This should not block this PR, let's track this in #546

colinfruit · 2019-11-27T00:09:18Z

add-on/src/popup/quick-upload.js

@@ -134,6 +134,8 @@ function file2buffer (file) {

 function formatImportDirectory (path) {
  path = path.replace(/\/$|$/, '/')
+  path = path.replace(/(\/)\/+/g, '$1')


I added this just because it makes a nicer user experience. Files will appear in the Web UI with multiple slashes (only on auto-open, from import page), despite being stored without them, if multiple slashes are not removed on format.

Thanks! Do you mind adding unit tests for formatImportDirectory, so we don't break this during future refactors?

For now it can be test/functional/lib/quick-upload.test.js (similar to existing test/functional/lib/ipfs-path.test.js)

lidel

One more ask: some users will prefer old behavior of opening at the gateway.
Please add a global setting Open imported files in Web UI to the "File Imports" section:

It should set the initial state of Open in Web UI checkbox on the quick-upload screen.

lidel · 2019-11-27T20:44:08Z

add-on/src/popup/quick-upload.js

+    // unless and embedded node is in use (no access to web UI)
+    // in which case, open resource.
+    if (state.ipfsNodeType === 'embedded' || !state.openViaWebUI) {
+      await ipfsCompanion.uploadResultHandler({ result, openRootInNewTab: true })


I believe the issue you described is due to slow content discovery by the gateway at https://ipfs.io/ipfs/.
Embedded node in Firefox/Chrome can't run own gateway, so we use public one instead.

Potential fix would be to update preloadAtPublicGateway and apart from existing HTTP HEAD request to user-specified public gateway, do additional recursive preload via HTTP GET request to one of preload nodes:

https://node0.preload.ipfs.io/api/v0/refs?arg={CID}&r=true https://node1.preload.ipfs.io/api/v0/refs?arg={CID}&r=true

In case of "quick-upload" screen, the CID should be of the wrapping directory.

This should not block this PR, let's track this in #546

lidel · 2019-11-27T20:57:24Z

add-on/src/lib/ipfs-companion.js

+    get openWebUiAtDirectory () {
+      return openWebUiAtDirectory
+    },


Embedded node is unable to open port to provide HTTP API, so the only option I see is to open WebUI from third party server, eg. https://webui.ipfs.io and make it use API of embedded node from Companion via window.ipfs. At the moment we don't support window.ipfs in WebUI because it is sandboxed, and WebUI requires full read-write access to MFS and config.
Details in: #514

lidel

@colinfruit Thanks! The UX of this is looking pretty nice ❤️ 👍

I believe we could go extra mile and switch everything to MFS, namely close #635 in this PR as well.

If you right click on image there will be context menu option to Add to IPFS.
The code responsible for that is in add-on/src/lib/ipfs-companion.js#onAddFromContext – do you think you could make it work the same as sharing screen?

copy to MFS instead of pinning
opening in WebUI instead of gateway
rename labels from Add to IPFS to Import to IPFS
- perhaps we should only keep the one that remembers the filename? CID-as-filename does not make much sense in MFS context, and CID can be copied via context menu anyway
respect global File Import preferences

Thoughts?

add-on/src/popup/quick-upload.js

…ipfs-companion This has been done using adding them to IPFS first (ipfs.add) first and then using the ipfs files copy api (ipfs.files.cp) to move the files into the desired directory. Please enter the commit message for your changes. Lines starting

Co-Authored-By: Marcin Rataj <lidel@lidel.org>

remove references to MFS and redundant 'import' messages Co-Authored-By: Marcin Rataj <lidel@lidel.org>

Co-Authored-By: Marcin Rataj <lidel@lidel.org>

This commit required abstracting the ipfs import functionality into a separate file to avoid a circular dependency between quick-upload and ipfs-companion, and continue to enable testing functionality.

lidel

LGTM, thank you @colinfruit!

Is there anything you want to add to this PR, or is it ok to merge & release to beta?

colinfruit · 2019-12-02T23:25:59Z

@lidel, fine to merge this. Thanks!

* language: replace upload with import terminology * update privacy policy language and last updated date Following up on the changes made in #810. This replaces references to `upload` with `import` Closes #817

lidel requested changes Nov 11, 2019

View reviewed changes

lidel reviewed Nov 12, 2019

View reviewed changes

lidel requested changes Nov 25, 2019

View reviewed changes

colinfruit commented Nov 26, 2019

View reviewed changes

colinfruit commented Nov 27, 2019

View reviewed changes

lidel requested changes Nov 27, 2019

View reviewed changes

lidel requested changes Nov 28, 2019

View reviewed changes

add-on/src/popup/quick-upload.js Outdated Show resolved Hide resolved

colinfruit and others added 19 commits November 30, 2019 18:12

upload files to MFS

3fdf5d7

format directory path properly on upload

6925e14

remove pinUpload description text

12ebff8

remove unnecessary comment

7d07ef3

move open web UI call out of file upload block

790421e

reword upload to import in UI messages

ae4db71

move default import directory to preferences screen

a67c7fa

open resource at CID if using embedded node

87ee1a0

add date symbols to import directory

7b0999a

add import options to import screen

23afca3

fix: only display openViaWebUI option if opening via web UI is possible

e816d3b

fix: wrap multiple files in a directory

b4bc09d

fix: remove wrapping directory from files API import

1f0f21b

group quickUpload messages together

7529b0a

add import page text input styling

788f694

Co-Authored-By: Marcin Rataj <lidel@lidel.org>

Update file import labels

a3ab203

remove references to MFS and redundant 'import' messages Co-Authored-By: Marcin Rataj <lidel@lidel.org>

pad dates in import directory

99e8073

Co-Authored-By: Marcin Rataj <lidel@lidel.org>

remove wrapWithDirectory option

5e85a93

colinfruit and others added 9 commits November 30, 2019 18:12

upload => import in MFS import variables

317c29a

trim double slashes from import directory

05ac606

add file import section on options page

31ebb13

move preloadAtPublicGateway option to file import section

55cef37

preload files when opening imported files in web UI

e0fe23b

test quick-upload formatImportDirectory

e4ba494

add openViaWebUI option to file import form

3ff1e69

Remove low-level pin when using ipfs.add

d9d1e9e

Co-Authored-By: Marcin Rataj <lidel@lidel.org>

Import to MFS from context menu

23259aa

This commit required abstracting the ipfs import functionality into a separate file to avoid a circular dependency between quick-upload and ipfs-companion, and continue to enable testing functionality.

lidel approved these changes Dec 2, 2019

View reviewed changes

lidel changed the title ~~feat: upload files to MFS~~ feat: import files to MFS Dec 3, 2019

lidel merged commit 92fd07a into ipfs:master Dec 3, 2019

This was referenced Dec 3, 2019

Files: shared link should include filename ipfs/ipfs-webui#1346

Closed

feat: sharing improvements ipfs/ipfs-webui#1347

Merged

Language: replace "upload" with "import" #817

Closed

colinfruit mentioned this pull request Dec 10, 2019

refactor: replace upload with import terminology #829

Merged

This was referenced Dec 12, 2019

Files: improve sharing flow from IPFS Companion #833

Closed

Feature: Add to ipfs with custom filename. #578

Closed

This was referenced Dec 24, 2019

Integration with ipfs-share-files #700

Closed

Display IPFS addresses instead of opening uploaded file #423

Closed

lidel mentioned this pull request Jun 11, 2021

Sharp (#) in url is not handled elegantly #989

Closed

lidel mentioned this pull request Aug 9, 2022

UX Bug: Add folder action is hidden behind "import" dropdown button ipfs/ipfs-webui#1951

Open

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: import files to MFS #810

feat: import files to MFS #810

colinfruit commented Nov 7, 2019 •

edited

Loading

lidel left a comment

lidel left a comment

lidel Nov 11, 2019

lidel Nov 12, 2019 •

edited

Loading

lidel Nov 14, 2019

colinfruit Nov 14, 2019 •

edited

Loading

lidel Nov 27, 2019

colinfruit commented Nov 15, 2019 •

edited

Loading

lidel commented Nov 19, 2019

lidel left a comment •

edited

Loading

lidel Nov 25, 2019 •

edited

Loading

colinfruit Nov 26, 2019

colinfruit commented Nov 26, 2019 •

edited

Loading

colinfruit Nov 26, 2019 •

edited

Loading

colinfruit Nov 26, 2019

lidel Nov 27, 2019

colinfruit Nov 27, 2019 •

edited

Loading

lidel Nov 27, 2019

lidel left a comment

lidel Nov 27, 2019

lidel Nov 27, 2019

lidel left a comment •

edited

Loading

lidel left a comment

colinfruit commented Dec 2, 2019

feat: import files to MFS #810

feat: import files to MFS #810

Conversation

colinfruit commented Nov 7, 2019 • edited Loading

lidel left a comment

Choose a reason for hiding this comment

lidel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidel Nov 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinfruit Nov 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinfruit commented Nov 15, 2019 • edited Loading

lidel commented Nov 19, 2019

lidel left a comment • edited Loading

Choose a reason for hiding this comment

lidel Nov 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinfruit commented Nov 26, 2019 • edited Loading

colinfruit Nov 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinfruit Nov 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidel left a comment • edited Loading

Choose a reason for hiding this comment

lidel left a comment

Choose a reason for hiding this comment

colinfruit commented Dec 2, 2019

colinfruit commented Nov 7, 2019 •

edited

Loading

lidel Nov 12, 2019 •

edited

Loading

colinfruit Nov 14, 2019 •

edited

Loading

colinfruit commented Nov 15, 2019 •

edited

Loading

lidel left a comment •

edited

Loading

lidel Nov 25, 2019 •

edited

Loading

colinfruit commented Nov 26, 2019 •

edited

Loading

colinfruit Nov 26, 2019 •

edited

Loading

colinfruit Nov 27, 2019 •

edited

Loading

lidel left a comment •

edited

Loading