binderhub icon indicating copy to clipboard operation
binderhub copied to clipboard

Rewrite the frontend

Open yuvipanda opened this issue 1 year ago • 3 comments

/* If this file gets over 200 lines of code long (not counting docs / comments), start using a framework

This was the first line I wrote when I started writing the existing frontend, and of course that line is still there.

This PR cleans up and rewrites the entire frontend, to 'start using a framework'. There's no functional change to the UI itself, so it should be treated as a pure upgrade / refactor.

Demo

Main page

https://github.com/jupyterhub/binderhub/assets/30430/4c629efb-2210-4f43-aeb3-3b3a0a4d65b8

Loading page

https://github.com/jupyterhub/binderhub/assets/30430/da6eefbe-8044-42aa-8562-03f7a79520ed

Functional Status

The following functional pieces need to be completed:

  • [x] Landing page link generator
  • [x] Loading page
  • [x] Actually launching servers correctly
  • [x] Progress bars for launching
  • [x] Stream logs
  • [x] Favicon switching to indicate progress
  • [x] About page
  • [x] Extra scripts at the bottom (for google analytics / similar)
  • [x] nbviewer in loading page
  • [x] Help text in the main page
  • [x] Banner on top
  • [x] Donation button on top
  • [x] Badge generator for markdown + rst
  • [x] view raw logs
  • [x] social cards
  • [ ] error page
  • [x] fix copy button icon
  • [ ] Faithfully replicate layout
  • [x] Fix markdown and rST icons

Technology changes

  • [x] Upgrades to Bootstrap 5, latest version. Nicely matches JupyterHub upgrade in version 5.
  • [x] Use react
  • [x] Use JSDoc type annotations rather than typescript. It feels to me this gives me a good balance between the positives of optional type checking without the extra community investment needed for full typescript. This PR does switch the compiler we use from babel to tsc, but type checking is not enforced. We may choose to do so later, but not now.
  • [x] Use react-router for URL parsing. This makes the BinderHub UI a SPA, which may be split into its own package separately in the future if so desired.
  • [x] Move the _config endpoint to a refactored /api/repoproviders. This is with an eye on allowing us to implement #844 eventually, as well as being able to implement the correct frontend bits in other users of the binderhub API (like https://github.com/yuvipanda/jupyterhub-fancy-profiles)
  • [x] Deprecate direct google analytics functionality, where we embedded GA code into our source. Instead, extra_footer_scripts can continue to be used - that's what we use for matomo.

Functionality changes

  • [x] Progress bar is now also shown in the loading page.
  • [x] Learning from experience with nbgitpuller, the link generator now prefers outputting only urlpath - both when the user enters a file to open or a URL to open. This prevents the issue we had when we tried to change the default app that was going to open from classic notebook to lab, and broke a lot of people's stuff. By only outputting urlpath, URLs will always have this information encoded in them. The older filepath and labpath are still accepted as input, because Cool URIs don't break
  • [x] Slightly better validation for the link generator, but most of this should be instead implemented as part of #844

Maintenance changes

One primary goal here is to make the frontend safer to change, so it's less fragile and brittle.

  • [ ] Add some frontend tests. This should be much easier now thanks to all the componentization.
  • [ ] Provide thorough jsdoc inline documentation for everything
  • [x] Refactor whatever is in binderhub-client package to make sure it only contains api-client related functionality - all UI stuff should be in a separate package.
  • [ ] Unify the current 'split' between the repo's root js/ and the binderhub/static/js sources of JS files. Pick up best practices from other Jupyterish projects for what to do here.

Timeline

My hope is to slowly but consistently work on this, and get it fully complete before end of June. I have also asked for some frontend review help from @batpad (either directly or via someone else), as he has significant experience in this kinda frontend work (even though he is less experienced in the JupyterHub community itself).

Fixes https://github.com/jupyterhub/binderhub/issues/774

yuvipanda avatar May 29 '24 20:05 yuvipanda

I've added functionality for the top banner here, and poked around to make sure that the existing banner can display well. It needs to be redone to use bootstrap 5 utility classes, but here it is:

        <div class="container-fluid position-relative" >
            <div>
                Thanks to <a href="https://www.ovh.com/">OVH</a>, <a href="https://notebooks.gesis.org">GESIS Notebooks</a> and <a href="https://curvenote.com">Curvenote</a> for supporting us! 🎉
                <br />
                mybinder.org has updated the base image to Ubuntu 22.04! See the <a href="https://repo2docker.readthedocs.io/en/latest/howto/breaking_changes.html">upgrade guide</a> for details.
            </div>

            <div class="top-0 end-0 position-absolute">
                <a class="btn" style="width:fit-content;height:fit-content;padding:10px;background-color:#e66581;color:white;font-weight:bold;"
                onmouseover="this.style.backgroundColor='#d15b75'" onmouseout="this.style.backgroundColor='#e66581'"
                href="https://numfocus.salsalabs.org/donate-to-binder" target="_blank">
                    🤍 Donate to mybinder.org!
                </a>
            </div>
        </div>

This works well!

yuvipanda avatar Jul 01 '24 22:07 yuvipanda

Have asked @oliverroick to do a review here since his React eyes are much, much sharper than mine.

batpad avatar Jul 11 '24 07:07 batpad

Getting closer and closer!

yuvipanda avatar Aug 02 '24 04:08 yuvipanda

We've finally got some dedicated time from @oliverroick so hopefully with that we can push this one to completion

yuvipanda avatar Nov 20 '24 23:11 yuvipanda

Will need to be rebased on https://github.com/jupyterhub/binderhub/pull/1891 but otherwise pretty close!

yuvipanda avatar Dec 07 '24 08:12 yuvipanda

I've merged #1891 here, and the failing tests are all actually genuine failures! \o/

yuvipanda avatar Dec 08 '24 06:12 yuvipanda

Looking at the build token and seeing how that is generated:

        build_token = jwt.encode(
            {
                "exp": int(time.time()) + self.settings["build_token_expires_seconds"],
                "aud": provider_spec,
                "origin": self.token_origin(),
            },
            key=self.settings["build_token_secret"],
            algorithm="HS256",
        )

It contains:

  1. Host
  2. Provider spec

While host is still available in the backend, as the PR is written, provider_spec is not available in the backend code. There are two paths to fixing this:

  1. Remove provider_spec from the jwt. This means the token will now be valid for all providers in the origin
  2. Change the handler so we do know what provider_spec is in the backend, and don't change the build token generator algorithm.

Reading the original PR that introduced it (https://github.com/jupyterhub/binderhub/pull/1309), my understanding is that this means that launching from the home page will be rate limited by IP, but launching from a link will not be rate limited by IP. I don't think I fully understand the considerations here, and will poke and ask for help!

yuvipanda avatar Dec 09 '24 03:12 yuvipanda

I don't recall super well, but the main thing was to limit cross-site launches, e.g. from thebe or API requests, but not from regular browser visits. I think you're right that the scheme may not be properly affecting home page builds, which I think isn't right, but also home page builds are a tiny fraction of launches in the current setup.

I think perhaps we may be able to get to a better, simpler result today using sec-fetch headers. The federation makes this complicated, but perhaps still manageable.

minrk avatar Dec 09 '24 07:12 minrk

Oh, I forgot to comment on the removal of jinja template support. I think most of the changes are AOK by me, but GESIS currently relies on this, but only to add some outer material. As I understand it, that should still work today. So instead of removing all documentation/support of templates, maybe we can update it to clarify that you can still re-skin page.html to some degree, but not reach into the 'App' itself, which I don't think anybody actually wants to do, and probalby doesn't really work today, anyway. WDYT? Or do you want to write new docs for how to accomplish these same goals in The New Way, e.g. with extra_header, extra_footer, extra_css (or theme_ etc.)?

minrk avatar Dec 09 '24 09:12 minrk

I've added support for build tokens! It still fails, but that's a genuine failure - I think it shows up because there was custom logic in https://github.com/jupyterhub/binderhub/blob/main/binderhub/static/js/src/form.js#L13 that we haven't really recreated anywhere. I don't want us to hardcode any provider stuff in the JS - so this needs to go into the displayConfig.

yuvipanda avatar Dec 09 '24 21:12 yuvipanda

Looks like we've kinda uncovered a rats nest of 'when do we expect repo to be encoded'? For example, with zenodo, we don't expect it to be encoded - localhost:8000/services/binder/v2/zenodo/10.5281/zenodo.3242074/ is a valid URI, but 10.5281/zenodo.3242074/ is the repo, even though it looks like repo/ref.

yuvipanda avatar Dec 09 '24 21:12 yuvipanda

The only times repo is actually uriEncoded is:

  1. When using gl provider (due to explicit check)
  2. When using git provider (as repo is a full http URL, and it matches the :// check)
  3. When using ckan provider (same :// check)

Everything else is unencoded, including when / are present (as it is for DOIs).

yuvipanda avatar Dec 09 '24 21:12 yuvipanda

The existing route is (r"/v2/([^/]+)/(.+)", ParameterizedMainHandler),. Basically I think we lucked out that repoproviders that aren't listed in https://github.com/jupyterhub/binderhub/pull/1856#issuecomment-2529602076 happen to have / in the repo part because DOIs have / in them! I don't think that was intentional.

yuvipanda avatar Dec 09 '24 22:12 yuvipanda

CC @rgaiacs https://github.com/jupyterhub/binderhub/pull/1856#issuecomment-2527354774

arnim avatar Dec 10 '24 10:12 arnim

I think in most cases we don't want to double-encode the spec unless we really have to (i.e. the spec is invalid as a URL path, as in git, or there is ambiguity as in gitlab), and in most cases we don't have to, because the spec is already defined by the repoprovider as a URL fragment. But you're right, we can only do that for repoproviders where the spec is a valid URL path fragment, either guaranteeing a certain number of / (github) or not needing to at all (DOI). For frontend purposes, it should be an explicit property of the provider whether the input to the resource / repo / etc. needs encoding or not.

The existing route is (r"/v2/([^/]+)/(.+)", ParameterizedMainHandler),. Basically I think we lucked out that repoproviders that aren't listed in https://github.com/jupyterhub/binderhub/pull/1856#issuecomment-2529602076 happen to have / in the repo part because DOIs have / in them! I don't think that was intentional.

I don't think so. The first group (([^/]+)) matches the repoprovider name (figshare or dataverse or gh), while the (.+) matches the full spec to pass to the provider, which may have zero or more slashes.

minrk avatar Dec 12 '24 09:12 minrk

For frontend purposes, it should be an explicit property of the provider whether the input to the resource / repo / etc. needs encoding or not.

Yep, this is now my implementation plan!

yuvipanda avatar Dec 12 '24 16:12 yuvipanda

yay! all tests pass!

yuvipanda avatar Dec 13 '24 16:12 yuvipanda

All this work is awesome! @arnim and I are planning to dedicated January 2025 to mybinder.org. We hope to make a huge progress and have things in a better state.

rgaiacs avatar Dec 13 '24 16:12 rgaiacs

@rgaiacs yay!

if we merge this as is, would that be ok with you? Or would we need to add some more customization features before we do that so we don't break your installs?

yuvipanda avatar Dec 13 '24 17:12 yuvipanda

@yuvipanda can we delay this to be merged on Tuesday? I'm finishing my day and I will not be able to provide support until Tuesday. Thanks for the understanding.

rgaiacs avatar Dec 13 '24 17:12 rgaiacs

@rgaiacs oh yeah, no problem at all!

yuvipanda avatar Dec 13 '24 17:12 yuvipanda

if we merge this as is, would that be ok with you?

@yuvipanda this should be OK. But I prefer if this could be merged on Tuesday for me to be ready to act if things stop on GESIS side.

Or would we need to add some more customization features before we do that so we don't break your installs?

@arnim and I need to do a frontend update on our installation because of GESIS change of corporate design. We are planning to do this in January and we will catch up with this changes.

rgaiacs avatar Dec 13 '24 17:12 rgaiacs

@rgaiacs ah excellent.

This still needs more review, so won't be merged before tuesday i think!

yuvipanda avatar Dec 13 '24 17:12 yuvipanda

The new interface is a lot narrower than the existing one, and the form background is lighter which I find makes it difficult to distinguish the input fields from the text labels, especially since the input fields have placeholder text that looks very similar to the label text. Do you think you could change the styling to make the input fields more obvious?

image image

manics avatar Dec 14 '24 22:12 manics

@manics the color contrast changes are the result of upgrading bootstrap. I do agree with you though, it could be better. I'll see what I can do.

nvm, i was wrong

yuvipanda avatar Dec 14 '24 23:12 yuvipanda

Screenshot 2024-12-14 at 6 02 16 PM

@manics this is how it looks now. what do you think?

While the form controls didn't flag in the WCAG Contrast Checker some other things did. I'll poke at those.

yuvipanda avatar Dec 15 '24 02:12 yuvipanda

After working on https://github.com/jupyterhub/repo2docker/pull/1393, I was able to get a better handle on what we actually support with hydroshare in binderhub and clarify that (only ids, not URLs). No change from before this PR, just clearer.

yuvipanda avatar Dec 21 '24 01:12 yuvipanda

Amazing effort into this @yuvipanda!!!! :heart: :tada:

consideRatio avatar Jan 03 '25 23:01 consideRatio

Woo, great job!

minrk avatar Jan 06 '25 07:01 minrk