Skip to content

Improve app and org loading states #16984

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

Merged
merged 13 commits into from
Mar 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions components/dashboard/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,31 @@
*/

import * as GitpodCookie from "@gitpod/gitpod-protocol/lib/util/gitpod-cookie";
import React, { FunctionComponent, Suspense } from "react";
import React, { FC, Suspense } from "react";
import { AppLoading } from "./app/AppLoading";
import { AppRoutes } from "./app/AppRoutes";
import { GitpodErrorBoundary } from "./components/ErrorBoundary";
import { useCurrentOrg } from "./data/organizations/orgs-query";
import { useAnalyticsTracking } from "./hooks/use-analytics-tracking";
import { useUserAndTeamsLoader } from "./hooks/use-user-and-teams-loader";
import { useUserLoader } from "./hooks/use-user-loader";
import { Login } from "./Login";
import { isGitpodIo } from "./utils";

const Setup = React.lazy(() => import(/* webpackPrefetch: true */ "./Setup"));

// Wrap the App in an ErrorBoundary to catch User/Org loading errors
// This will also catch any errors that happen to bubble all the way up to the top
const AppWithErrorBoundary: FC = () => {
return (
<GitpodErrorBoundary>
<App />
</GitpodErrorBoundary>
);
};

// Top level Dashboard App component
const App: FunctionComponent = () => {
const { user, isSetupRequired, loading } = useUserAndTeamsLoader();
const App: FC = () => {
const { user, isSetupRequired, loading } = useUserLoader();
const currentOrgQuery = useCurrentOrg();

// Setup analytics/tracking
Expand Down Expand Up @@ -67,4 +78,4 @@ const App: FunctionComponent = () => {
);
};

export default App;
export default AppWithErrorBoundary;
15 changes: 13 additions & 2 deletions components/dashboard/src/app/AppLoading.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,19 @@
*/

import { FunctionComponent } from "react";
import { Delayed } from "../components/Delayed";
import { Heading3, Subheading } from "../components/typography/headings";
import gitpodIcon from "../icons/gitpod.svg";

// TODO: Would be great to show a loading UI if it's been loading for awhile
export const AppLoading: FunctionComponent = () => {
return <></>;
return (
// Wait 2 seconds before showing the loading screen to avoid flashing it too quickly
<Delayed wait={2000}>
<div className="flex flex-col justify-center items-center w-full h-screen space-y-4">
<img src={gitpodIcon} alt="Gitpod's logo" className={"h-16 flex-shrink-0"} />
<Heading3>Just getting a few more things ready</Heading3>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: I like the playful message, but what do you think of going with something simpler and less verbose, like a spinner with no text, using the Loader component?

BEFORE AFTER
Screenshot 2023-03-23 at 11 20 36 224318369-368eaa86-4bc1-4bfa-a18b-5cab5f3f775d

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love something that has some personality to it. Also, that small loading spinner is so easy to miss, I don't even realize it's there. Ideally most people don't see this, but for those that do, it'd be great to brighten the experience a little w/ something that could make them smile.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could improve this in a followup once we find something actionable.

IMO the PR is already a biiig improvement over the status quo 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll drop the bouncing animation for now to reduce the chance of confusion w/ the workspace start flow, and we can follow up w/ any design/copy updates.

<Subheading>hang in there...</Subheading>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: If we're keeping the loading message, I'd go with something less verbose and drop the heading above, as this page does not need it.

</div>
</Delayed>
);
};
26 changes: 26 additions & 0 deletions components/dashboard/src/components/Delayed.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/**
* Copyright (c) 2023 Gitpod GmbH. All rights reserved.
* Licensed under the GNU Affero General Public License (AGPL).
* See License.AGPL.txt in the project root for license information.
*/

import { FC, useEffect, useState } from "react";

type Props = {
wait?: number;
};
export const Delayed: FC<Props> = ({ wait = 1000, children }) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Not sure if this is the best line to comment about this or if this is already implementing the following: Ideally, we would wait a second or two before showing the full-page loading indicator to avoid having spinners or loading states everywhere. 💡

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I think the same thing. This component is built so we can use it for those loading indicators too, and just wrap them with this. We can do it in the components themselves so we get it for free everywhere..

const [show, setShow] = useState(false);

useEffect(() => {
const timeout = setTimeout(() => setShow(true), wait);
return () => clearTimeout(timeout);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

if (!show) {
return null;
}

return <>{children}</>;
};
6 changes: 3 additions & 3 deletions components/dashboard/src/components/EmptyMessage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@
*/

import classNames from "classnames";
import { FC, useCallback } from "react";
import { FC, ReactNode, useCallback } from "react";
import { Button } from "./Button";
import { Heading2, Subheading } from "./typography/headings";

type Props = {
title: string;
subtitle?: string;
title: ReactNode;
subtitle?: ReactNode;
buttonText?: string;
onClick?: () => void;
className?: string;
Expand Down
20 changes: 18 additions & 2 deletions components/dashboard/src/components/ErrorBoundary.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,18 @@ export const GitpodErrorBoundary: FC = ({ children }) => {
);
};

type CaughtError = Error & { code?: number };

export const DefaultErrorFallback: FC<FallbackProps> = ({ error, resetErrorBoundary }) => {
// adjust typing, as we may have caught an api error here w/ a code property
const caughtError = error as CaughtError;

const emailSubject = encodeURIComponent("Gitpod Dashboard Error");
const emailBody = encodeURIComponent(`\n\nError: ${error.message}`);
let emailBodyStr = `\n\nError: ${caughtError.message}`;
if (caughtError.code) {
emailBodyStr += `\nCode: ${caughtError.code}`;
}
const emailBody = encodeURIComponent(emailBodyStr);

return (
<div role="alert" className="app-container mt-14 flex flex-col items-center justify-center space-y-6">
Expand All @@ -36,7 +45,14 @@ export const DefaultErrorFallback: FC<FallbackProps> = ({ error, resetErrorBound
<div>
<button onClick={resetErrorBoundary}>Reload</button>
</div>
{error.message && <pre>{error.message}</pre>}
<div>
{caughtError.code && (
<span>
<strong>Code:</strong> {caughtError.code}
</span>
)}
{caughtError.message && <pre>{caughtError.message}</pre>}
</div>
</div>
);
};
Expand Down
17 changes: 5 additions & 12 deletions components/dashboard/src/data/organizations/orgs-query.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,16 @@
*/

import { Organization, OrgMemberInfo, User } from "@gitpod/gitpod-protocol";
import { BillingMode } from "@gitpod/gitpod-protocol/lib/billing-mode";
import { useQuery, useQueryClient } from "@tanstack/react-query";
import { useCallback, useContext } from "react";
import { useCallback } from "react";
import { useLocation } from "react-router";
import { publicApiTeamMembersToProtocol, publicApiTeamToProtocol, teamsService } from "../../service/public-api";
import { getGitpodService } from "../../service/service";
import { useCurrentUser, UserContext } from "../../user-context";
import { useCurrentUser } from "../../user-context";
import { getUserBillingModeQueryKey } from "../billing-mode/user-billing-mode-query";
import { noPersistence } from "../setup";

export interface OrganizationInfo extends Organization {
members: OrgMemberInfo[];
billingMode?: BillingMode;
isOwner: boolean;
invitationId?: string;
}
Expand All @@ -34,7 +31,6 @@ export function useOrganizationsInvalidator() {
export function useOrganizations() {
const user = useCurrentUser();
const queryClient = useQueryClient();
const { refreshUserBillingMode } = useContext(UserContext);
const query = useQuery<OrganizationInfo[], Error>(
getQueryKey(user),
async () => {
Expand All @@ -47,13 +43,11 @@ export function useOrganizations() {
const response = await teamsService.listTeams({});
const result: OrganizationInfo[] = [];
for (const org of response.teams) {
const billingMode = await getGitpodService().server.getBillingModeForTeam(org.id);
const members = publicApiTeamMembersToProtocol(org.members || []);
const isOwner = members.some((m) => m.role === "owner" && m.userId === user?.id);
result.push({
...publicApiTeamToProtocol(org),
members,
billingMode,
isOwner,
invitationId: org.teamInvitation?.id,
});
Expand All @@ -65,16 +59,15 @@ export function useOrganizations() {
if (!user) {
return;
}

// refresh user billing mode to update the billing mode in the user context as it depends on the orgs
refreshUserBillingMode();
queryClient.invalidateQueries(getUserBillingModeQueryKey(user.id));
},
onError: (err) => {
console.error("useOrganizations", err);
},
enabled: !!user,
cacheTime: 1000 * 60 * 60 * 1, // 1 hour
staleTime: 1000 * 60 * 60 * 1, // 1 hour
// We'll let an ErrorBoundary catch the error
useErrorBoundary: true,
},
);
return query;
Expand Down
11 changes: 9 additions & 2 deletions components/dashboard/src/data/setup.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
PersistQueryClientProvider,
PersistQueryClientProviderProps,
} from "@tanstack/react-query-persist-client";
import { QueryClient, QueryKey } from "@tanstack/react-query";
import { QueryCache, QueryClient, QueryKey } from "@tanstack/react-query";
import { ReactQueryDevtools } from "@tanstack/react-query-devtools";
import { FunctionComponent } from "react";

Expand All @@ -28,7 +28,14 @@ export function isNoPersistence(queryKey: QueryKey): boolean {
}

export const setupQueryClientProvider = () => {
const client = new QueryClient();
const client = new QueryClient({
queryCache: new QueryCache({
// log any errors our queries throw
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡

onError: (error) => {
console.error(error);
},
}),
});
const queryClientPersister = createIDBPersister();

const persistOptions: PersistQueryClientProviderProps["persistOptions"] = {
Expand Down
5 changes: 5 additions & 0 deletions components/dashboard/src/hooks/use-analytics-tracking.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ export const useAnalyticsTracking = () => {

// listen and notify Segment of client-side path updates
useEffect(() => {
//store current path to have access to previous when path changes
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this into here instead of it being in the user-loader hook for some reason.

const w = window as any;
const _gp = w._gp || (w._gp = {});
_gp.path = window.location.pathname;

return history.listen((location: any) => {
const path = window.location.pathname;
trackPathChange({
Expand Down
45 changes: 0 additions & 45 deletions components/dashboard/src/hooks/use-user-and-teams-loader.ts

This file was deleted.

58 changes: 58 additions & 0 deletions components/dashboard/src/hooks/use-user-loader.ts
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed this to just use-user-loader since it doesn't deal with teams anymore. Contents also changed to wrap call in a useQuery().

Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/**
* Copyright (c) 2022 Gitpod GmbH. All rights reserved.
* Licensed under the GNU Affero General Public License (AGPL).
* See License.AGPL.txt in the project root for license information.
*/

import { useState, useContext } from "react";
import { User } from "@gitpod/gitpod-protocol";
import { UserContext } from "../user-context";
import { getGitpodService } from "../service/service";
import { ErrorCodes } from "@gitpod/gitpod-protocol/lib/messaging/error";
import { trackLocation } from "../Analytics";
import { refreshSearchData } from "../components/RepositoryFinder";
import { useQuery } from "@tanstack/react-query";
import { noPersistence } from "../data/setup";

export const useUserLoader = () => {
const { user, setUser } = useContext(UserContext);
const [isSetupRequired, setSetupRequired] = useState(false);

// For now, we're using the user context to store the user, but letting react-query handle the loading
// In the future, we should remove the user context and use react-query to access the user
const { isLoading } = useQuery({
queryKey: noPersistence(["current-user"]),
queryFn: async () => {
let user: User | undefined;
try {
user = await getGitpodService().server.getLoggedInUser();
setUser(user);
refreshSearchData();
} catch (error) {
if (error && "code" in error) {
if (error.code === ErrorCodes.SETUP_REQUIRED) {
setSetupRequired(true);
return;
}

// If it was a server error, throw it so we can catch it with an ErrorBoundary
if (error.code >= 500) {
throw error;
}

// Other errors will treat user as needing to log in
}
} finally {
trackLocation(!!user);
}

return user || null;
},
// We'll let an ErrorBoundary catch the error
useErrorBoundary: true,
cacheTime: 1000 * 60 * 60 * 1, // 1 hour
staleTime: 1000 * 60 * 60 * 1, // 1 hour
});

return { user, loading: isLoading, isSetupRequired };
};
7 changes: 3 additions & 4 deletions components/dashboard/src/menu/OrganizationSelector.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { BillingMode } from "@gitpod/gitpod-protocol/lib/billing-mode";
import { useUserBillingMode } from "../data/billing-mode/user-billing-mode-query";
import { useFeatureFlags } from "../contexts/FeatureFlagContext";
import { useCurrentOrg, useOrganizations } from "../data/organizations/orgs-query";
import { useOrgBillingMode } from "../data/billing-mode/org-billing-mode-query";

export interface OrganizationSelectorProps {}

Expand All @@ -20,6 +21,7 @@ export default function OrganizationSelector(p: OrganizationSelectorProps) {
const orgs = useOrganizations();
const currentOrg = useCurrentOrg();
const { data: userBillingMode } = useUserBillingMode();
const { data: orgBillingMode } = useOrgBillingMode();
const { showUsageView } = useFeatureFlags();

const userFullName = user?.fullName || user?.name || "...";
Expand Down Expand Up @@ -70,10 +72,7 @@ export default function OrganizationSelector(p: OrganizationSelectorProps) {
BillingMode.showUsageBasedBilling(userBillingMode) &&
!user?.additionalData?.isMigratedToTeamOnlyAttribution;

const showUsageForOrg =
currentOrg.data &&
currentOrg.data.isOwner &&
(currentOrg.data.billingMode?.mode === "usage-based" || showUsageView);
const showUsageForOrg = currentOrg.data?.isOwner && (orgBillingMode?.mode === "usage-based" || showUsageView);

if (showUsageForPersonalAccount || showUsageForOrg) {
linkEntries.push({
Expand Down
Loading