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.

Sign up for GitHub

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

Jump to bottom

Connection status handling #1150

Merged

mason-fish merged 1 commit into master from 1099-sync_zqd_post_init

Oct 21, 2020

Contributor

mason-fish commented Oct 15, 2020

fixes #1099

Changes

Introduce connection statuses, stored in the clusters state.
Persist spaces state.
Initialization no longer depends on zqd and instead the data syncs come from the loaded app components

Every time the app loads we reset all statuses to "initial", indicating that we are unsure if the connection will succeed yet, and also indicating that we need to sync with server data. While syncing, if the connection fails we set the status to "disconnected" and display as much in a new tab content page where the user my attempt to retry.

Signed-off-by: Mason Fish mason@looky.cloud

mason-fish force-pushed the 1099-sync_zqd_post_init branch from 6945a0d to 5994439 Compare

October 15, 2020 16:12

mason-fish requested a review from jameskerr

October 15, 2020 16:56

jameskerr requested changes

View reviewed changes

Member

jameskerr left a comment

Right on! I like the code. I'm glad we talked and thought about it for a minute. Thanks for deleting all that old login stuff too.

In addition to the suggestions below, here are a few thoughts.

Yellow Notice

Now that we have this connection error view, the yellow notice that says "Failed to fetch" is unnecessary. I think it should not appear in this case (when we're testing out a connection).

This might be more difficult than it seems. We pop up that "Notice" if any request in the whole app throws. You'll have to see if there is a way to remove the error handler only here, or if we need to remove it from where it is now and place it only where it's needed.

Wording of notice

As Al suggested, I think the wording of this error message needs to be updated to:

The service at {host}:{port} could not be reached.

src/js/components/ConnectionError.tsx

+                flex-direction: column;
+              `
+              const StyledHeader = styled.h1`

Member

jameskerr Oct 15, 2020

This text style is called "heading/page" in sketch. Will you add it to the styled components theme then include it here.

Contributor Author

mason-fish Oct 15, 2020

sure thing

src/js/components/ConnectionError.tsx Outdated

+                margin: 110px 0 0 0;
+                font-size: 24px;
+                font-weight: 700;
+                color: ${(p) => p.theme.colors.aqua};

Member

jameskerr Oct 15, 2020

Any objection to using color: var(--aqua) ?

src/js/components/ConnectionError.tsx Outdated

+                const onClick = async () => {
+                  setIsFetching(true)
+                  await retry()
+                  console.log("done")

Member

jameskerr Oct 15, 2020

Suggested change

console.log("done")

src/js/components/ConnectionError.tsx Outdated

Comment on lines 25 to 26

		font-size: 13px;
		font-weight: 400;

Member

jameskerr Oct 15, 2020

Suggested change

      
              font-size: 13px;
          
              font-weight: 400;
          
              ${(p) => p.theme.typography.labelNormal}

src/js/components/ConnectionError.tsx Outdated

+                const onClick = async () => {
+                  setIsFetching(true)
+                  await retry()

Member

jameskerr Oct 15, 2020

It might sound backward, but would you add a slight delay before actually retrying. Just long enough so that we're guaranteed to see the spinner. This will make it feel more responsive. If the spinner never appears, the user will wonder... did I click it?

I suggest here 300ms but play with value to see what feels good.

Suggested change

      
                await retry()
          
                await new Promise((r) => setTimeout(r, 300)
          
                await retry()

Contributor Author

mason-fish Oct 15, 2020

ah interesting! I had put in a wait just to test that it worked properly but then took it back out. I noticed the missing feedback then and that's why I added the Notice to the retry method, but if we end up removing that then I def agree that some sort of feedback is important

src/js/flows/setConnection.ts

		@@ -13,13 +13,15 @@ export const setConnection = (cluster: Cluster) => (
		return zealot

Member

jameskerr Oct 15, 2020

Maybe this file needs to be renamed to initConnection, similar to how we have initSpace and initTab

src/js/initializers/initConnectionParams.ts Outdated

               const setupConnection = (host, port) => (dispatch, _, {globalDispatch}) => {
-                const cluster = {
+                console.log("setup connection running")

Member

jameskerr Oct 15, 2020

Suggested change

console.log("setup connection running")

src/js/initializers/initConnectionParams.ts

                 }
                 dispatch(Clusters.add(cluster))
                 globalDispatch(Clusters.add(cluster))
                 dispatch(Current.setConnectionId(cluster.id))
               }
-              export default async function(store: Store) {
-                const {space, host, port, id} = getUrlSearchParams()
+              export default function(store: Store) {

Member

jameskerr Oct 15, 2020

+1 to simpler code here

src/js/state/Clusters/flows.ts

-              import Viewer from "../Viewer"
-              import {getZealot} from "../../flows/getZealot"
-              export function connectCluster(cluster: Cluster): Thunk<Promise<void>> {

Member

jameskerr Oct 15, 2020

❤️ that this all is gone

src/js/state/Clusters/types.ts Outdated

		}

		export type Status = "initial" \| "connected" \| "disconnected"

Member

jameskerr Oct 15, 2020

Might be better to call this ConnectionStatus since we have other places we use the word Status. (SearchStatus, ViewerStatus, ChartStatus, UidStatus...etc)

mason-fish force-pushed the 1099-sync_zqd_post_init branch 5 times, most recently from 21897c4 to 7ce03e3 Compare

October 19, 2020 21:14

mason-fish mentioned this pull request

Connection names #1157

Merged

mason-fish force-pushed the 1099-sync_zqd_post_init branch 6 times, most recently from 0a8afc2 to 1a81b6c Compare

October 20, 2020 19:22

jameskerr approved these changes

View reviewed changes

Member

jameskerr left a comment

Looks awesome. I like the delay, works very well.

src/js/components/ClusterPicker.tsx

    
                      dispatch(setConnection(c)).catch((e) => {

                        dispatch(Notice.set(e))

                      dispatch(initConnection(c)).catch(() => {

                        dispatch(Current.setConnectionId(c.id))

Member

jameskerr Oct 20, 2020

Oh nice! Easy

mason-fish force-pushed the 1099-sync_zqd_post_init branch from 1a81b6c to 94cd1c3 Compare

October 21, 2020 17:18


          add and track connection status, use components for app load

ccf1347

Signed-off-by: Mason Fish <mason@looky.cloud>

mason-fish force-pushed the 1099-sync_zqd_post_init branch from 94cd1c3 to ccf1347 Compare

October 21, 2020 17:21

mason-fish merged commit 8636e29 into master

mason-fish deleted the 1099-sync_zqd_post_init branch

October 21, 2020 17:43

philrz linked an issue

that may be closed by this pull request

Crash when no current space and network error #1048

Closed

philrz mentioned this pull request

Additional Brim windows show spinners #1161

Closed

philrz mentioned this pull request

Opening Log Detail window causes main window to spin (commit 8636e29) #1183

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment