Implement new App Launcher Not Running Page/Functionality #384

jbouder · 2024-07-11T20:09:31Z

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)
Refactoring (no functional changes, no API changes)
Build related changes
Other (please describe):

Testing

Did you test the pull request locally?
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

…ew not running page to app.

vercel · 2024-07-11T20:09:36Z

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

jbouder · 2024-07-11T20:19:47Z

jhub_apps/service/japps_routes.py

@@ -23,7 +23,7 @@ async def handle_apps(request: Request):
    if not theme:
        theme = themes.DEFAULT_THEME
    return templates.TemplateResponse(
-        "japps_home.html",
+        "japps_custom.html",


Note, this page template is only used by pages which require a custom route through the japps service. This change was just to reduce confusion with naming between this template and the real base template (japps_page)

jbouder · 2024-07-11T20:21:39Z

jhub_apps/service/routes.py

+    # If server_name is 'lab' then it is the default user
+    if server_name == "lab" or server_name == "vscode":
+        server_name = ""
+
+    if server_name is not None:


This was updated to allow calling this endpoint from the default server (JupyterLab or VSCode). Just passing an empty server_name string today results in the endpoint returning all apps, which is not what we want here.

jbouder · 2024-07-11T20:22:44Z

jhub_apps/templates/style.css

+.card-dialog-body-wrapper {
+    padding: 0 24px 20px !important;
+}
+


Fix spacing issue in modal dialogs, which are currently being overridden by JupyterLab styling

jbouder · 2024-07-11T20:23:39Z

ui/src/components/context-menu/context-menu.tsx

@@ -125,6 +125,7 @@ export const ContextMenu = ({
              }}
              onClick={(e) => {
                if (!item.disabled && item.onClick) {
+                  e.stopPropagation();


This prevents the onclick event from bubbling up when the context menu should actually be handling the event

jbouder · 2024-07-11T20:25:23Z

ui/src/pages/not-running/not-running.tsx

+  useEffect(() => {
+    if (currentUser) {
+      const currentId = window.location.pathname
+        .replace(/\/$/, '')


Trim any extra slashes from the end to ensure we get a proper split on strings

jbouder · 2024-07-11T20:26:20Z

ui/src/utils/jupyterhub.ts

@@ -93,6 +97,7 @@ export const getApps = (
        ...server,
        id: app.name,
        name: app.display_name,
+        url: server.url?.replace('/user/', '/hub/user/'),


Fixes 404 error when clicking on stopped app

kildre

It looks good to me, tests passing and coverage good. No errors.

aktech · 2024-07-15T14:06:55Z

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

jbouder · 2024-07-15T14:10:09Z

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

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

aktech · 2024-07-15T14:15:05Z

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

Yep, everything else seems to be running fine.

jbouder · 2024-07-15T14:15:54Z

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 · 2024-07-16T08:46:12Z

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

aktech · 2024-07-16T09:26:09Z

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.

jbouder · 2024-07-16T11:27:21Z

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 · 2024-07-16T11:45:03Z

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 · 2024-07-16T11:53:25Z

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

aktech

Looks great to me. Verified the click context, works as expected.

jbouder added 7 commits July 10, 2024 10:10

Updates to generalize base templates. Add not running template. Add n…

f15fd32

…ew not running page to app.

Update to redirect and display modal when not started.

b39b4b4

Fix modal title. Fix modal styling.

ca9846c

Refactor urls into utils.

319753a

Update to display not running dialog when apps ready and card clicked.

554f0fd

Update to track app to start in session storage.

fafa445

Add todo.

d32a00f

vercel bot deployed to Preview – jhub-apps-docs July 11, 2024 20:09 View deployment

vercel bot deployed to Preview – jhub-apps July 11, 2024 20:10 View deployment

jbouder commented Jul 11, 2024

View reviewed changes

Add and update unit tests.

853ba51

vercel bot deployed to Preview – jhub-apps-docs July 11, 2024 22:30 View deployment

vercel bot deployed to Preview – jhub-apps July 11, 2024 22:31 View deployment

Merge branch 'main' into not-running-updates

60ec168

vercel bot deployed to Preview – jhub-apps-docs July 12, 2024 13:41 View deployment

vercel bot deployed to Preview – jhub-apps July 12, 2024 13:42 View deployment

kildre approved these changes Jul 12, 2024

View reviewed changes

Add progress indicator to not running page.

8e08525

vercel bot deployed to Preview – jhub-apps-docs July 12, 2024 19:26 View deployment

vercel bot deployed to Preview – jhub-apps July 12, 2024 19:27 View deployment

Remove unneeded server status alert.

ff716b4

vercel bot deployed to Preview – jhub-apps-docs July 15, 2024 14:33 View deployment

vercel bot deployed to Preview – jhub-apps July 15, 2024 14:33 View deployment

Merge branch 'main' into not-running-updates

51d2fcd

vercel bot deployed to Preview – jhub-apps July 15, 2024 21:16 View deployment

vercel bot deployed to Preview – jhub-apps-docs July 15, 2024 21:17 View deployment

Fix context menu showing server not showing when click away.

e6ed248

vercel bot deployed to Preview – jhub-apps July 16, 2024 11:44 View deployment

vercel bot deployed to Preview – jhub-apps-docs July 16, 2024 11:44 View deployment

aktech approved these changes Jul 16, 2024

View reviewed changes

aktech merged commit 563b8df into nebari-dev:main Jul 16, 2024
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement new App Launcher Not Running Page/Functionality #384

Implement new App Launcher Not Running Page/Functionality #384

jbouder commented Jul 11, 2024 •

edited

Loading

vercel bot commented Jul 11, 2024 •

edited

Loading

jbouder Jul 11, 2024

jbouder Jul 11, 2024

jbouder Jul 11, 2024

jbouder Jul 11, 2024

jbouder Jul 11, 2024

jbouder Jul 11, 2024

kildre left a comment

aktech commented Jul 15, 2024

jbouder commented Jul 15, 2024

aktech commented Jul 15, 2024

jbouder commented Jul 15, 2024

aktech commented Jul 16, 2024

aktech commented Jul 16, 2024

jbouder commented Jul 16, 2024

jbouder commented Jul 16, 2024

jbouder commented Jul 16, 2024

aktech left a comment

Implement new App Launcher Not Running Page/Functionality #384

Implement new App Launcher Not Running Page/Functionality #384

Conversation

jbouder commented Jul 11, 2024 • edited Loading

Reference Issues or PRs

What does this implement/fix?

Testing

Screenshots

Any other comments?

vercel bot commented Jul 11, 2024 • edited Loading

jbouder Jul 11, 2024

Choose a reason for hiding this comment

jbouder Jul 11, 2024

Choose a reason for hiding this comment

jbouder Jul 11, 2024

Choose a reason for hiding this comment

jbouder Jul 11, 2024

Choose a reason for hiding this comment

jbouder Jul 11, 2024

Choose a reason for hiding this comment

jbouder Jul 11, 2024

Choose a reason for hiding this comment

kildre left a comment

Choose a reason for hiding this comment

aktech commented Jul 15, 2024

jbouder commented Jul 15, 2024

aktech commented Jul 15, 2024

jbouder commented Jul 15, 2024

aktech commented Jul 16, 2024

aktech commented Jul 16, 2024

jbouder commented Jul 16, 2024

jbouder commented Jul 16, 2024

jbouder commented Jul 16, 2024

aktech left a comment

Choose a reason for hiding this comment

jbouder commented Jul 11, 2024 •

edited

Loading

vercel bot commented Jul 11, 2024 •

edited

Loading