-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Changes from all commits
bd1b1a9
dd59c9b
8be6221
a6ee8fd
c7cd864
5b49d99
2b2efab
5612b74
8ffb53b
2b5c434
dd937d8
c500450
0b1a677
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> | ||
<Subheading>hang in there...</Subheading> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
); | ||
}; |
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 }) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. 💡 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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}</>; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"; | ||
|
||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 |
||
onError: (error) => { | ||
console.error(error); | ||
}, | ||
}), | ||
}); | ||
const queryClientPersister = createIDBPersister(); | ||
|
||
const persistOptions: PersistQueryClientProviderProps["persistOptions"] = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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({ | ||
|
This file was deleted.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renamed this to just |
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"]), | ||
geropl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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 }; | ||
}; |
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😉
There was a problem hiding this comment.
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.