-
Notifications
You must be signed in to change notification settings - Fork 489
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
Conversation
There was a problem hiding this 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!
There was a problem hiding this 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 ofpin remote
functionality:- if it works and returns a list (could be empty), trigger rendering of the list
- if it errors (eg. because backend is go-ipfs older than 0.7.0) then disable "Add service" button and replace onboarding text with some error message.
- @jessicaschilling we probably should prepare new onboarding text displayed above the list of pinning services when
pin remote
commands are present. Right now the user is presented with:
https://github.com/ipfs-shipyard/ipfs-webui/blob/53ebbce1ebae2ea0b483eebf45d71a81c068ccf8/public/locales/en/settings.json#L8
We could keep this for situation whenpin.remote
is not available, but we need new one for when it is present.
- @jessicaschilling we probably should prepare new onboarding text displayed above the list of pinning services when
- I believe we should execute
- 💚 Adding a new service works as expected and after that I see newly added one + pre-existing ones I had already defined via CLI:
- 🤔 e2e tests fail with 0.8.0-rc1 but I can take a look at them after we fix regular
npm test
which fail withTypeError: TextDecoder is not a constructor
(which seems to be due to some changes inipfs-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)
We have that text already in an earlier branch ... somewhere. I'll dig it out on Monday. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this 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?
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 I think we could hide Auto Upload column for now and merge it to unblock #1721 |
@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. |
@rafaelramalho19 quick notes on remaining work here:
|
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 🙏 |
Co-authored-by: Marcin Rataj <lidel@lidel.org>
Co-authored-by: Marcin Rataj <lidel@lidel.org>
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.
b0c18d7
to
a19a5ef
Compare
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.
@rafaelramalho19 good news, I was able to fix those random errors: 🥳
Was not able to crash it since then, good sign :) Remaining actionable UX pains which you might be able to pick up:
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):
|
The modal PR is waiting for this, since it's not even functional without the proper API for unpinning files
Unsure what you mean about this, since everytime we open the pinning modal we fetch the remote pin services again ( |
What I meant is that we should disable checkbox next to services that are known to be offline. |
- 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
- sets icon and docs url when name includes template name
There was a problem hiding this 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 😊
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) .. and Files: TODO (can be filled as separate issue):
|
Co-authored-by: Jessica Schilling <jessica@protocol.ai>
Co-authored-by: Jessica Schilling <jessica@protocol.ai>
Co-authored-by: Jessica Schilling <jessica@protocol.ai>
There was a problem hiding this 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 👍
Description
Testing
To test this locally, follow these steps:
npm ci
.connect-deps link /path/to/js-ipfs/packages/ipfs-http-client
and thenconnect-deps connect
if successful, you should be able to run
npm i && npm start
and have getIpfs().pin.remote