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

Valid sub-routes for the UI app result in 404 #899

Closed
gabalafou opened this issue Oct 18, 2024 · 8 comments · Fixed by #932
Closed

Valid sub-routes for the UI app result in 404 #899

gabalafou opened this issue Oct 18, 2024 · 8 comments · Fixed by #932
Assignees
Labels

Comments

@gabalafou
Copy link
Contributor

gabalafou commented Oct 18, 2024

Should we redirect all unknown routes to the React (UI) app?

When a user goes directly to a React app-defined sub-route (by sub-route I mean any route that is not the root route, "/"), such as "/:namespace/new-environment", the server will return a 404 instead of returning the conda-store-ui.html page.

We should probably redirect all unknown routes to the React (UI) app, conda-store-ui.

That way a user can copy-paste a link like "https://example.com/conda-store/default/new-environment" directly into their browser, as opposed to always having to start from "https://example.com/conda-store". Being able to load non-root routes was the whole point of introducing URL routing into the front end.

@gabalafou gabalafou changed the title Redirect all unknown routes to the React app Redirect all unknown routes to the UI app Oct 18, 2024
@gabalafou gabalafou mentioned this issue Oct 18, 2024
3 tasks
@gabalafou gabalafou changed the title Redirect all unknown routes to the UI app Valid sub-routes for the UI app result in 404 Oct 18, 2024
@gabalafou gabalafou added needs: triaging 🚦 Someone needs to have a look at this issue and triage type: bug 🐛 Something isn't working labels Oct 21, 2024
@trallard trallard added area: user experience 👩🏻‍💻 Items impacting the end-user experience area: api 🌐 and removed needs: triaging 🚦 Someone needs to have a look at this issue and triage labels Oct 21, 2024
@trallard
Copy link
Collaborator

Does this mean that to get to let's say namespace/environments right now the user has to go through / then find their way through clicks to namespace/environments?

@gabalafou
Copy link
Contributor Author

Yes that's exactly the issue

@trallard
Copy link
Collaborator

Pffff that sounds awful. So I would be in favour of finding a better way to deal with routes.

@gabalafou
Copy link
Contributor Author

gabalafou commented Oct 21, 2024

Exactly. The current configuration defeats the whole point of adding URL routing to the UI app. Part of the argument for getting rid of the tabbed interface was that a user could ctrl- or cmd-click on environments to open them in a new browser tab rather than a new Conda Store UI tab. But if we can't load the UI at URLs like /:namespace/:environment, then that ability to open environments directly in a new tab (or share them with others via URL) does not work.

I believe the usual solution here is to create catch-all or fallback route on the server that serves conda-store-ui.html. Right now, I believe the server is configured to serve that file at one and only one URL: "/" by default, or "/conda-store" in other configurations.

@trallard
Copy link
Collaborator

I assigned you @gabalafou and @peytondmurray - I would like you two to figure a solution to this.

@peytondmurray
Copy link
Contributor

For my understanding, when a user pastes https://example.com/conda-store/foo/bar

  • currently they get a 404
  • under the proposed change, we redirect to https://example.com/conda-store, then window.location gets parsed to take the user to the bar environment in the foo namespace

Is that right?

@gabalafou
Copy link
Contributor Author

Yes! Except it's not technically a redirect. It's not a 301. It's a 200 and you serve the conda-store-ui.html template at /foo/bar.

@peytondmurray
Copy link
Contributor

peytondmurray commented Oct 24, 2024

Cool, thanks for the clarification - that solution sounds reasonable to me.

@gabalafou Feel free to pick this up as time permits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants