Implement new App Launcher Not Running Page/Functionality

nebari-dev / jhub-apps

Application creator and general launcher for JupyterHub

https://jhub-apps.nebari.dev/

BSD 3-Clause "New" or "Revised" License

28 stars 12 forks source link

Implement new App Launcher Not Running Page/Functionality #384

Closed jbouder closed 4 months ago

jbouder commented 4 months ago

Reference Issues or PRs

N/A

What does this implement/fix?

Put a x in the boxes that apply

[ ] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds a feature)
[ ] Breaking change (fix or feature that would cause existing features not to work as expected)
[ ] Documentation Update
[ ] Code style update (formatting, renaming)
[x] Refactoring (no functional changes, no API changes)
[ ] Build related changes
[ ] Other (please describe):

Testing

[x] Did you test the pull request locally?
[x] Did you add new tests?

Screenshots

Any other comments?

Updated to base page templates to simplify future legacy migrations
Added new Not Running page and modal to React App
Updated to redirect to home and open modal when app isn't started and user navigates to stopped app directly
Updated app cards to display Not Running modal on click when not started

Note: native browser-based session storage is being used to track redirect and open ux. This will be replaced with global state when app is running as full SPA

vercel[bot] commented 4 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
jhub-apps	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 16, 2024 11:44am
jhub-apps-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 16, 2024 11:44am

aktech commented 4 months ago

I am seeing this on a local nebari deployment, any ideas? @jbouder

jbouder commented 4 months ago

I am seeing this on a local nebari deployment, any ideas? @jbouder

Aside from this message, is the app loading as expected?

aktech commented 4 months ago

Aside from this message, is the app loading as expected?

Yep, everything else seems to be running fine.

jbouder commented 4 months ago

Aside from this message, is the app loading as expected?

Yep, everything else seems to be running fine.

Cool. I think there is an overly specific check thats adding that message. I'll get an update pushed shortly.

aktech commented 4 months ago

It looks like the modal appears even when I just got off to clicking on the context menu, seems weird IMO, is that expected behaviour?

https://github.com/user-attachments/assets/5b5c32f7-1ccd-4f4c-8c92-b95265580176

aktech commented 4 months ago

We would need to handle the case when a shared app is stopped and the person with whom the app is shared, tried to access the app and then start it, in that case "starting the app" will fail. At the moment backend returns 500, but after this is merged https://github.com/nebari-dev/jhub-apps/pull/397 it will return 404.

We're not allowing the shared user to start the app and in future we may, in that case it will return either 200 or 403.

So to summarise we need to handle the following cases:

200 (already handled)
404, 403: The error message should be something like you don't have permissions to start user "original-author"'s app, please ask the app author to start the app. (Same error for both cases since we're not allowing app starts by non-author for now)
500: Something went wrong, generic error.

The later two cases are not handled at the moment. I am happy to get this PR in and handle those in a follow up PR as this is a huge improvement anyway.

jbouder commented 4 months ago

It looks like the modal appears even when I just got off to clicking on the context menu, seems weird IMO, is that expected behaviour?

Screen.Recording.2024-07-16.at.9.43.49.am.mov

Good catch. Since we're adding a click event to the card, I need to make sure the events on the card and the context menu do not conflict. I fixed that in one place, but missed this one. Will get that fixed.

jbouder commented 4 months ago

We would need to handle the case when a shared app is stopped and the person with whom the app is shared, tried to access the app and then start it, in that case "starting the app" will fail. At the moment backend returns 500, but after this is merged #397 it will return 404.

We're not allowing the shared user to start the app and in future we may, in that case it will return either 200 or 403.

So to summarise we need to handle the following cases:

200 (already handled)

404, 403: The error message should be something like you don't have permissions to start user "original-author"'s app, please ask the app author to start the app. (Same error for both cases since we're not allowing app starts by non-author for now)

500: Something went wrong, generic error.

The later two cases are not handled at the moment. I am happy to get this PR in and handle those in a follow up PR as this is a huge improvement anyway.

I think we can handle this separately as it may take a little more work. Can you create an issue with the specifics?

jbouder commented 4 months ago

It looks like the modal appears even when I just got off to clicking on the context menu, seems weird IMO, is that expected behaviour? Screen.Recording.2024-07-16.at.9.43.49.am.mov

Good catch. Since we're adding a click event to the card, I need to make sure the events on the card and the context menu do not conflict. I fixed that in one place, but missed this one. Will get that fixed.

Fixed