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: remote pinning settings page #1713

Merged
merged 26 commits into from
Mar 26, 2021
Merged

Conversation

rafaelramalho19
Copy link
Contributor

@rafaelramalho19 rafaelramalho19 commented Jan 21, 2021

Description

Testing

To test this locally, follow these steps:

  • Checkout js-ipfs into local dir and switch to @Gozala branch (feat/pin-remote)
  • Go to js-ipfs/package/ipfs-http-client and try to build it via npm ci.
  • Go back to ipfs-webui dir, and execute connect-deps link /path/to/js-ipfs/packages/ipfs-http-client and then connect-deps connect
    if successful, you should be able to run npm i && npm start and have getIpfs().pin.remote

Copy link
Contributor

@jessicaschilling jessicaschilling left a comment

Choose a reason for hiding this comment

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

LGTM (based on the demo video - let me know if I need to dig deeper) with only one request: that the colors allocated to services' icons remain consistent. Thank you!

@lidel lidel changed the title Feature/remote pinning settings page feat: remote pinning settings page Jan 25, 2021
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 @rafaelramalho19!

I made a quick pass and wrote down some nits, so we don't miss them:

  • 💔 When I open Settings for the first time it does not show me my pre-existing services.
    • I believe we should execute pin.remote.service.ls on initial page load as a quick test of pin remote functionality:
  • 💚 Adding a new service works as expected and after that I see newly added one + pre-existing ones I had already defined via CLI:
    Screenshot_2021-01-25 Settings IPFS
  • 🤔 e2e tests fail with 0.8.0-rc1 but I can take a look at them after we fix regular npm test which fail with TypeError: TextDecoder is not a constructor (which seems to be due to some changes in ipfs-http-client) – don't worry about those yet, we will circle back after wiring of Files screen is finished
  • 🤔 Clicking on "Set pinning" on Files screen fails with TypeError: remoteServices is not iterable
    (not a big deal, but if you will work on Files in separate PR, then would be cool to stub it here for now, so it does not break, and we can emerge this PR independently)

src/bundles/pinning.js Outdated Show resolved Hide resolved
src/components/pinning-manager/PinningManager.js Outdated Show resolved Hide resolved
src/components/pinning-manager/PinningManager.js Outdated Show resolved Hide resolved
@jessicaschilling
Copy link
Contributor

We have that text already in an earlier branch ... somewhere. I'll dig it out on Monday.

@lidel

This comment has been minimized.

@rafaelramalho19

This comment has been minimized.

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.

We merged latest http-client with #1655 so connect-deps should no longer be necessary.

@rafaelramalho19 mind rebasing this PR?

@lidel lidel added the P1 High: Likely tackled by core team if no one steps up label Feb 25, 2021
@lidel
Copy link
Member

lidel commented Mar 4, 2021

I've merged fixes from master and switched Pinata endpoint to production – seems there were no bugs, things work as expected.

@jessicaschilling mind trying to build this branch locally when you have some spare time? regular npm ci && npm start should just work (no need for connect-deps anymore)

I think we could hide Auto Upload column for now and merge it to unblock #1721
(MFS autopinning can be added in a separate PR later).

@lidel lidel self-requested a review March 4, 2021 15:04
@jessicaschilling
Copy link
Contributor

@lidel looks good to me - I don't have a Pinata account to test on, but shoot me details OOB if you want me to specifically test. Agree that removing the auto upload column to push the feature out faster is a good move.

@lidel
Copy link
Member

lidel commented Mar 15, 2021

@rafaelramalho19 quick notes on remaining work here:

  • Files screen still breaks when one of your pinning services is offline (screenshot)
  • As you have bandwidth now, please wire up Auto Upload column to toggle Enable flag in MFS Pining policy
    • this can be done via ipfs.config, on the CLI it works like this:
      ipfs config --json Pinning.RemoteServices.your-service.Policies.MFS.Enable false
      
  • On my test node Settings page fails to calculate their size due to goPinsSizeGet error. I am not sure if error is due to that, or something else.
    • I have 1k of local pins and multiple remote services – unlikely the cause, but mentioning for the record.
    • Some of remote services are broken (offline) – perhaps this is the cause?
  • (Assuming previous bug is fixed) Calling files.stat for each CID in goPinsSizeGet is too slow to be execute every time Settings screen is loaded. some CIDs may represent huge DAGs and calculating size of each takes time
    • Try adding a localStorage-backed cache that memoizes the cumulativeSize of each CID, so files.stat is called only once. Maybe that is enough.
  • Fill a separate issue to revert d20e4e3 and investigate why JS fails (so we dont forget)

@rafaelramalho19
Copy link
Contributor Author

On my test node Settings page fails to calculate their size due to goPinsSizeGet error. I am not sure if error is due to that, or something else.

This was due to ipfs not being ready at the time of trying to fetch the services, I changed how that works, see if it's fixed now 🙏

Using pre-existing stat helper makes webui immune to unexpected
exceptions.

Pin size calculation was removed, because it was already hidden in GUI
of Settings screen.
@lidel lidel force-pushed the feature/remote-pinning-settings-page branch from b0c18d7 to a19a5ef Compare March 23, 2021 22:38
This fixes a racy bug: when a single service was offline,
any remaining services were not checked due to error thrown.

This usually did not happen in Chromium, but failed pretty often in
Firefox, which seem to execute looped for awaits bit differently.
@lidel
Copy link
Member

lidel commented Mar 24, 2021

@rafaelramalho19 good news, I was able to fix those random errors: 🥳

  • Made size calculations immune to unexpected error + disabled pin size calculation in b0c18d7
  • 5d6a1b4 fixed a bug when if one of services returned error for pin.remote.ls (that stopped fetching remote pins from remaining services) – this was specific to Firefox and more than 4 services, so hard to replicate.

Was not able to crash it since then, good sign :)

Remaining actionable UX pains which you might be able to pick up:

  • disable offline services in pinning modal
    I think we may avoid all sorts of problems if "Set pinning" modal executes ipfs pin remote service ls --stat check (just like we do on Settings) and disables services that are not online, so user can't pin to service that is offline and avoids error.

  • delete modal does not unpin locally by default

Things that I am unsure how to tackle (would appreciate thoughts/ideas):

Text updates (ok to ignore for now, writing down so we don't forget before merge):

@rafaelramalho19
Copy link
Contributor Author

delete modal does not unpin locally by default
We track this in #1644, is that PR waiting for this one to be merged, or is it other way around?

The modal PR is waiting for this, since it's not even functional without the proper API for unpinning files

disable offline services in pinning modal

Unsure what you mean about this, since everytime we open the pinning modal we fetch the remote pin services again (doFetchPinningServices) which updates the list, or am I missing something?

@lidel
Copy link
Member

lidel commented Mar 25, 2021

What I meant is that we should disable checkbox next to services that are known to be offline.
(And fade inactive ones out a bit, so user can tell those are not available for pinning)

- basic explainer whatpolicy change means
- enable/disable
- manual/all files
- filled missing labels with basic explainer
  (can be improved in separate PRs)
- disabled pinning to services that are offline + added visual
  indication when there is a problem with service
@lidel
Copy link
Member

lidel commented Mar 25, 2021

Pushed some changes:

  • filled missing labels with basic explainer (canbe improved in separate PRs)
  • disabled pinning to services that are offline + added visual indication when there is a problem with service

Screenshot_2021-03-25  animals pets Files IPFS
Screenshot_2021-03-25 Settings IPFS
Screenshot_2021-03-25 Settings IPFS(1)

- sets icon and docs url when name includes template name
Copy link
Contributor

@jessicaschilling jessicaschilling left a comment

Choose a reason for hiding this comment

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

Swooped in with some small text edits ... hope that's OK 😊

public/locales/en/settings.json Outdated Show resolved Hide resolved
public/locales/en/settings.json Outdated Show resolved Hide resolved
@lidel
Copy link
Member

lidel commented Mar 25, 2021

Fixed icon/link support for remote service templates in GUI. (We can decide to diable it later, but should look pretty good when people have only one service)
If name includes template name (for now we only have "pinata" one), then icon + link to docs from template are used on Settings:

2021-03-25--22-06-21

.. and Files:

2021-03-25--22-07-07

TODO (can be filled as separate issue):

  • store only CID in template, and use same gateway for loading the icon as FilePreviews on Files screen

lidel and others added 2 commits March 25, 2021 22:12
Co-authored-by: Jessica Schilling <jessica@protocol.ai>
Co-authored-by: Jessica Schilling <jessica@protocol.ai>
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 @rafaelramalho19 for everything that went into this one ❤️
As this PR was initially about Settings, I believe it is functionally complete and good to merge to unblock #1644.

I extracted the remaining work into separate issues and put them in milestone v2.12.
@rafaelramalho19 when you feel better, feel free to pic tasks from mentioned milestone, starting with #1644. Remember to self-assign, so we don't duplicate work.

We are nearly there 👍

@lidel lidel merged commit e3bf9c8 into master Mar 26, 2021
@lidel lidel deleted the feature/remote-pinning-settings-page branch March 26, 2021 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 High: Likely tackled by core team if no one steps up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants