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: upload progress ui #1655

Merged
merged 14 commits into from
Feb 25, 2021
Merged

feat: upload progress ui #1655

merged 14 commits into from
Feb 25, 2021

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Sep 30, 2020

This pull request is a draft that contains more changes than it should, namely it fixes some of the storybook stories that I appeared broken, I will factor out those changes into separate PR.

This pull request adds stories corresponding to the mockups at https://www.figma.com/file/BQjzJcaKuNrNgeDnoZ664D/Web-UI-upload%2Fdownload-progress

image

image

image

image

image

image

@jessicaschilling I was not very successful in styling such that there will be no whitespace below progressbar, with what tachyons provides. If you could share some tips that would be greatly appreciated.


Now the bad news is (as @rafaelramalho19 mentioned it during the call) react is unable to catch up with all the updates that are happening to put it very gently. Some profiling showed an extreme high pressure on both CPU. On my macbook Air it's 0fps, in fact total block time time (TBT) which should be kept <50ms was whooping 86412ms 😱. Put it differently while uploading 1gig file app was able to render only two frames.

image

I did some instrumentation and here are some early findings:

  1. There are way too many re-renders happening. I am not sure yet where these updates coming from, but backlog of re-renders is so massive that chrome console is unable to store only the ones that happen when progress is at 0. Which suggests that those updates aren't caused by file add itself.

  2. I can see state updates happening on file add. While state is updated to progress 32% react is still trying to process re-renders queue of updates where state is still 0%.

    It is ridiculous that react still does re-render on every update instead of doing it on animation frames BTW, something that other libraries had been doing for a long time.


I am afraid this is not going to be easy, because scope of this problem is well beyond specific components. I'll need to investigate this further before I can tell if there are some shortcuts we could take, but it may well require significant changes to get a reasonable re-render loop.

@Gozala Gozala self-assigned this Sep 30, 2020
@Gozala Gozala marked this pull request as draft September 30, 2020 00:56
@jessicaschilling
Copy link
Contributor

Boo, I suspected tachyons' weird height unit incrementing was going to foul that up. Feel free to explicitly give the progress bar itself a normal-measurement height.

@Gozala
Copy link
Contributor Author

Gozala commented Sep 30, 2020

Turns out all the progress updates start firing only after the request is complete, and that caused the flood of updates that react was unable to keep up with. I'll try to figure out why updates are starting to come in only after request is complete.

@Gozala
Copy link
Contributor Author

Gozala commented Oct 2, 2020

With the following changes applied:

I finally was able to resolve all the issues and get a progress bar working

Screen Recording 2020-10-02 at 12 03 15 PM 2020-10-02 12_10_09

@Gozala Gozala changed the base branch from master to storybook-fixes October 2, 2020 20:14
@Gozala
Copy link
Contributor Author

Gozala commented Oct 2, 2020

Cleaned up pull request and to remove unrelated changes and re-targeted it to storybook-fixes branch instead as those changes are needed to get the storybook stuff working here. Once that branch lands I'll change target back to master.

Please note that in order to test any of these changes you'd need to manually update ipfs-http-client dependency to include changes mentioned earlier. Unfortunately it does not appears like there is a way to do it with git urls because of the monorepo setup.

@Gozala
Copy link
Contributor Author

Gozala commented Oct 2, 2020

Fixed the styling so there is no whitespace below the progressbar and that we only display it if import is pending. I think it's ready to be reviewed even if it will take some time to get all the dependencies to propagate before it can land.

Base automatically changed from storybook-fixes to master October 6, 2020 16:18
if (loaded > uploadState.progress) {
uploadState = { progress: loaded, entries }
yield uploadState
}
}

try {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing catch handling for this try. If, for example, a file already exists with the name you're trying to upload, it fails and crashes the whole app.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rafaelramalho19 that is an issue, but I don't think it was introduced by changes here. Can we address it separately ? We could also probably ship fix to that sooner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a separate issue for that #1669

@Gozala
Copy link
Contributor Author

Gozala commented Jan 20, 2021

Status update

I have run into problems in updating this pull. For whatever reason "react scripts" seems to modify isolatedModules to true which than causes issues with new ipfs-http-client (which we need to get progress events):

Failed to compile.

/Users/gozala/Projects/ipfs-webui/node_modules/ipfs-core-types/src/index.ts
TypeScript error in /Users/gozala/Projects/ipfs-webui/node_modules/ipfs-core-types/src/index.ts(5,3):
Re-exporting a type when the '--isolatedModules' flag is provided requires using 'export type'.  TS1205

    3 | 
    4 | export {
  > 5 |   RootAPI,
      |   ^
    6 | 
    7 |   AbortOptions,
    8 |   Await,

This is really annoying and I was unable to get it resolved yet. I hope we won't have to ship changes in http-client to fix this, although I am starting to run out of ideas. Even if I delete tsconfig.json react scripts seems to create one and produce above error.

@Gozala Gozala marked this pull request as ready for review January 20, 2021 23:35
@Gozala
Copy link
Contributor Author

Gozala commented Jan 20, 2021

Worked around the issue for now

@Gozala
Copy link
Contributor Author

Gozala commented Jan 21, 2021

Now we're running into Gozala/web-encoding#1

@lidel
Copy link
Member

lidel commented Jan 26, 2021

Updating CI to the latest LTS Node (14.15.4) fixed one of tests:

  • 12.x: Tests: 4 failed, 22 passed, 26 total
  • 14.x: Tests: 3 failed, 23 passed, 26 total

@lidel
Copy link
Member

lidel commented Feb 10, 2021

@Gozala any luck here? Can you switch to ipfs-http-client 49.x ?
I'd love to land this and then rebase #1713 and #1721 to include fixes from this PR.

@Gozala
Copy link
Contributor Author

Gozala commented Feb 22, 2021

I figured out why I was not able to reproduce the problems on my device - I was using yarn which is deduplicating dependencies, with npm I'm able to replicate the issue locally. I am also getting bunch of issues caused by multiple versions of same dependecy. Trying to get versions aligned now & hope it will get it all fixed.

@Gozala
Copy link
Contributor Author

Gozala commented Feb 23, 2021

Identified problem which is caused by the following change:
ipfs/js-ipfs@f243dd1#diff-451c33c54d61ab1f32e767b878ccd03a507ba13b852934fd40d32472df2f81f2R76-R78

Specifically ipfs.ls(emptyDir) contains CID of the emptyDir now, while prior to this change it returned no entries.

@Gozala
Copy link
Contributor Author

Gozala commented Feb 23, 2021

Submitted an issue ipfs/js-ipfs#3566

@Gozala
Copy link
Contributor Author

Gozala commented Feb 23, 2021

I am trying to figure what is causing the remote-api.test failures, so far without much success. So far I have noticed that if I just have one test enabled it passes (regardless of the test). If I add bunch of alerts in the code things pass as well.

All this leads me believe that there is some timing problem possibly with with starting helper servers, but all this combined makes it really difficult.

This makes E2E tests for remote API less flaky:
- leverage expect-puppeteer where possible
- tweak navigation so it is not impacted by connection-error state
- force refresh of status page to avoid wait for manual refresh
js-ipfs changed CLI path at some point recently
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.

I fixed e2e tests in fc4b2dc and 39eaccc, but its only a testament we should move to playwright at some point.

CI is green, LGTM.

I tested import of 800MB ZIM file, and it looks ok.

@lidel lidel changed the title draft: upload progress ui feat: upload progress ui Feb 25, 2021
@lidel lidel self-requested a review February 25, 2021 01:38
@lidel
Copy link
Member

lidel commented Feb 25, 2021

@Gozala is there anything left in this PR that you'd like to do before we merge?

I don't want to block this on those commented tests, perhaps we merge this to unblock rebase of Rafael's PRs and fill issue for fixing tests in separate PR?

@Gozala
Copy link
Contributor Author

Gozala commented Feb 25, 2021

@Gozala is there anything left in this PR that you'd like to do before we merge?

I don't want to block this on those commented tests, perhaps we merge this to unblock rebase of Rafael's PRs and fill issue for fixing tests in separate PR?

Can’t thank you enough for helping with CI issues! I think merging and doing everything as followups is the best way to go

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 @Gozala for all the time that went into this ❤️ 🚀
(not just this PR, but even more work happened upstream in js-ipfs libs)

I'm merging this so we can rebase all the other PRs on top of the latest http-client and fixed CI/tests.

@lidel lidel merged commit f236694 into master Feb 25, 2021
@lidel lidel deleted the import-progress branch February 25, 2021 14:23
rafaelramalho19 pushed a commit that referenced this pull request Mar 12, 2021
This PR adds visual feedback for then big files are imported:
progress bar + % status.

There is also new http-client and a bunch of required fixes.

* fix: broken file list stories
* feat: upload progress ui
* chore: integrate external changes
* fix: integrate new http client
* fix: jsdom problem
* fix(cid) switch circleci to new images
* fix(e2e): test/e2e/remote-api.test.js
  This makes E2E tests for remote API less flaky:
  - leverage expect-puppeteer where possible
  - tweak navigation so it is not impacted by connection-error state
  - force refresh of status page to avoid wait for manual refresh
  * fix(ci): E2E_IPFSD_TYPE=js npm run test:e2e
  js-ipfs changed CLI path at some point recently

Co-authored-by: Marcin Rataj <lidel@lidel.org>
rafaelramalho19 pushed a commit that referenced this pull request Mar 15, 2021
This PR adds visual feedback for then big files are imported:
progress bar + % status.

There is also new http-client and a bunch of required fixes.

* fix: broken file list stories
* feat: upload progress ui
* chore: integrate external changes
* fix: integrate new http client
* fix: jsdom problem
* fix(cid) switch circleci to new images
* fix(e2e): test/e2e/remote-api.test.js
  This makes E2E tests for remote API less flaky:
  - leverage expect-puppeteer where possible
  - tweak navigation so it is not impacted by connection-error state
  - force refresh of status page to avoid wait for manual refresh
  * fix(ci): E2E_IPFSD_TYPE=js npm run test:e2e
  js-ipfs changed CLI path at some point recently

Co-authored-by: Marcin Rataj <lidel@lidel.org>
rafaelramalho19 added a commit that referenced this pull request Mar 15, 2021
* feat: upload progress ui (#1655)

This PR adds visual feedback for then big files are imported:
progress bar + % status.

There is also new http-client and a bunch of required fixes.

* fix: broken file list stories
* feat: upload progress ui
* chore: integrate external changes
* fix: integrate new http client
* fix: jsdom problem
* fix(cid) switch circleci to new images
* fix(e2e): test/e2e/remote-api.test.js
  This makes E2E tests for remote API less flaky:
  - leverage expect-puppeteer where possible
  - tweak navigation so it is not impacted by connection-error state
  - force refresh of status page to avoid wait for manual refresh
  * fix(ci): E2E_IPFSD_TYPE=js npm run test:e2e
  js-ipfs changed CLI path at some point recently

Co-authored-by: Marcin Rataj <lidel@lidel.org>

* chore: rollback connect-deps

* chore: go-ipfs 0.8.0-rc1

* feat: integrate remote pinning in the files page

* chore: update deps

* chore: linting fixes

* feat: add pinning services to the pinning modal

* chore: update conflicts

Co-authored-by: Irakli Gozalishvili <contact@gozala.io>
Co-authored-by: Marcin Rataj <lidel@lidel.org>
rafaelramalho19 added a commit that referenced this pull request Mar 19, 2021
* feat: upload progress ui (#1655)

This PR adds visual feedback for then big files are imported:
progress bar + % status.

There is also new http-client and a bunch of required fixes.

* fix: broken file list stories
* feat: upload progress ui
* chore: integrate external changes
* fix: integrate new http client
* fix: jsdom problem
* fix(cid) switch circleci to new images
* fix(e2e): test/e2e/remote-api.test.js
  This makes E2E tests for remote API less flaky:
  - leverage expect-puppeteer where possible
  - tweak navigation so it is not impacted by connection-error state
  - force refresh of status page to avoid wait for manual refresh
  * fix(ci): E2E_IPFSD_TYPE=js npm run test:e2e
  js-ipfs changed CLI path at some point recently

Co-authored-by: Marcin Rataj <lidel@lidel.org>

* chore: rollback connect-deps

* chore: go-ipfs 0.8.0-rc1

* feat: integrate remote pinning in the files page

* chore: update deps

* chore: linting fixes

* feat: add pinning services to the pinning modal

* chore: update conflicts

Co-authored-by: Irakli Gozalishvili <contact@gozala.io>
Co-authored-by: Marcin Rataj <lidel@lidel.org>
lidel added a commit that referenced this pull request Mar 26, 2021
* chore: WIP in settings page
* feat: add remote pinning services to the settings page
* chore: memoise pinning service color
* chore: set the same colors for pinning services manager items
* chore: update src/components/pinning-manager/PinningManager.js
* chore: remove console.log
* chore: make pinning work in go-ipfs 0.7 & add docs link
* chore: move pinning constants to another file
* feat: detect if remote services are available in the settings page
* chore: switch to production Pinata
https://pinata.cloud/documentation#PinningServicesAPI

* feat: remote pins on files page (#1721)
* feat: upload progress ui (#1655)

This PR adds visual feedback for when big files are imported:
progress bar + % status.

There is also new http-client and a bunch of required fixes.

* fix: broken file list stories
* feat: upload progress ui
* chore: integrate external changes
* fix: integrate new http client
* fix: jsdom problem
* fix(cid) switch circleci to new images
* fix(e2e): test/e2e/remote-api.test.js
  This makes E2E tests for remote API less flaky:
  - leverage expect-puppeteer where possible
  - tweak navigation so it is not impacted by connection-error state
  - force refresh of status page to avoid wait for manual refresh
  * fix(ci): E2E_IPFSD_TYPE=js npm run test:e2e
  js-ipfs changed CLI path at some point recently

Co-authored-by: Marcin Rataj <lidel@lidel.org>

* chore: rollback connect-deps

* chore: go-ipfs 0.8.0-rc1

* feat: integrate remote pinning in the files page

* chore: update deps

* chore: linting fixes

* feat: add pinning services to the pinning modal

* chore: update conflicts

Co-authored-by: Irakli Gozalishvili <contact@gozala.io>
Co-authored-by: Marcin Rataj <lidel@lidel.org>

* chore: disable js-ipfs tests for the time being

* feat: add autoUpload toggle to the pinning manager

* chore: change autoupload labels

* refactor: simplify size calculations

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.

* fix: pin.remote.ls status check

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.

* fix: meaningful labels and variable names

* fix: auto upload labels

- basic explainer whatpolicy change means
- enable/disable
- manual/all files

* fix: disable pinning to offline services

- 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

* fix: pinning service templates

- sets icon and docs url when name includes template name

* style: auto upload modal title

Co-authored-by: Jessica Schilling <jessica@protocol.ai>

* style: auto upload modal description

Co-authored-by: Jessica Schilling <jessica@protocol.ai>

* style: auto upload menu item

Co-authored-by: Jessica Schilling <jessica@protocol.ai>

* fix: no online check for local pinning

* style: separate title for remote pin glyph

* chore: cleanup

Co-authored-by: Marcin Rataj <lidel@lidel.org>
Co-authored-by: Irakli Gozalishvili <contact@gozala.io>
Co-authored-by: Jessica Schilling <jessica@protocol.ai>
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.

4 participants