Skip to content

[dashboard] Fix feedback when triggering prebuilds #5731

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

Closed
wants to merge 2 commits into from
Closed

[dashboard] Fix feedback when triggering prebuilds #5731

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3456d2c
[dashboard] Fix spinner
jankeromnes Sep 17, 2021
ceba53e
[dashboard] Disable & add spinner to button when triggering a new pre…
jankeromnes Sep 21, 2021
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
2 changes: 1 addition & 1 deletion components/dashboard/src/icons/Spinner.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 0 additions & 1 deletion components/dashboard/src/icons/SpinnerDark.svg
View file
Open in desktop

This file was deleted.

5 changes: 2 additions & 3 deletions components/dashboard/src/projects/ConfigureProject.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import { getGitpodService } from "../service/service";
import { getCurrentTeam, TeamsContext } from "../teams/teams-context";
import Header from "../components/Header";
import Spinner from "../icons/Spinner.svg";
import SpinnerDark from "../icons/SpinnerDark.svg";
import PrebuildLogsEmpty from "../images/prebuild-logs-empty.svg";
import PrebuildLogsEmptyDark from "../images/prebuild-logs-empty-dark.svg";
import { ThemeContext } from "../theme-context";
Expand Down Expand Up @@ -163,7 +162,7 @@ export default function () {
<MonacoEditor classes="w-full flex-grow" disabled={isEditorDisabled} language="dockerfile" value={dockerfile} onChange={setDockerfile} />}
</Suspense>
{isDetecting && <div className="absolute h-full w-full bg-gray-100 dark:bg-gray-700 flex items-center justify-center space-x-2">
<img className="h-5 w-5 animate-spin" src={isDark ? SpinnerDark : Spinner} />
<img className="h-5 w-5 animate-spin" src={Spinner} />
<span className="font-semibold text-gray-400">Detecting project configuration ...</span>
</div>}
</div>
Expand All @@ -177,7 +176,7 @@ export default function () {
</div>)
}</div>
<div className="h-20 px-6 bg-gray-50 dark:bg-gray-800 border-t border-gray-200 dark:border-gray-600 flex space-x-2">
{prebuildWasTriggered && <PrebuildInstanceStatus prebuildInstance={prebuildInstance} isDark={isDark} />}
{prebuildWasTriggered && <PrebuildInstanceStatus prebuildInstance={prebuildInstance} />}
<div className="flex-grow" />
{(prebuildInstance?.status.phase === "stopped" && !prebuildInstance?.status.conditions.failed)
? <a className="my-auto" href={`/#${project?.cloneUrl}`}><button className="secondary">New Workspace</button></a>
Expand Down
4 changes: 1 addition & 3 deletions components/dashboard/src/projects/Prebuild.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import Header from "../components/Header";
import PrebuildLogs from "../components/PrebuildLogs";
import { getGitpodService, gitpodHostUrl } from "../service/service";
import { TeamsContext, getCurrentTeam } from "../teams/teams-context";
import { ThemeContext } from "../theme-context";
import { PrebuildInstanceStatus } from "./Prebuilds";
import { shortCommitMessage } from "./render-utils";

Expand All @@ -28,7 +27,6 @@ export default function () {

const [ prebuild, setPrebuild ] = useState<PrebuildWithStatus | undefined>();
const [ prebuildInstance, setPrebuildInstance ] = useState<WorkspaceInstance | undefined>();
const { isDark } = useContext(ThemeContext);

useEffect(() => {
if (!teams || !projectName || !prebuildId) {
Expand Down Expand Up @@ -88,7 +86,7 @@ export default function () {
<PrebuildLogs workspaceId={prebuild?.info?.buildWorkspaceId} onInstanceUpdate={onInstanceUpdate} />
</div>
<div className="h-20 px-6 bg-gray-50 dark:bg-gray-800 border-t border-gray-200 dark:border-gray-600 flex space-x-2">
{prebuildInstance && <PrebuildInstanceStatus prebuildInstance={prebuildInstance} isDark={isDark} />}
{prebuildInstance && <PrebuildInstanceStatus prebuildInstance={prebuildInstance} />}
<div className="flex-grow" />
{prebuildInstance?.status.phase === "stopped"
? <a className="my-auto" href={gitpodHostUrl.withContext(`${prebuild?.info.changeUrl}`).toString()}><button>New Workspace</button></a>
Expand Down
33 changes: 24 additions & 9 deletions components/dashboard/src/projects/Prebuilds.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,13 @@
*/

import moment from "moment";
import { PrebuildInfo, PrebuildWithStatus, PrebuiltWorkspaceState, Project, WorkspaceInstance } from "@gitpod/gitpod-protocol";
import { PrebuildInfo, PrebuildWithStatus, PrebuiltWorkspaceState, Project, StartPrebuildResult, WorkspaceInstance } from "@gitpod/gitpod-protocol";
import { useContext, useEffect, useState } from "react";
import { useHistory, useLocation, useRouteMatch } from "react-router";
import Header from "../components/Header";
import DropDown, { DropDownEntry } from "../components/DropDown";
import { ItemsList, Item, ItemField, ItemFieldContextMenu } from "../components/ItemsList";
import Spinner from "../icons/Spinner.svg";
import SpinnerDark from "../icons/SpinnerDark.svg";
import StatusDone from "../icons/StatusDone.svg";
import StatusFailed from "../icons/StatusFailed.svg";
import StatusPaused from "../icons/StatusPaused.svg";
Expand All @@ -38,6 +37,7 @@ export default function () {
const [statusFilter, setStatusFilter] = useState<PrebuiltWorkspaceState | undefined>();

const [prebuilds, setPrebuilds] = useState<PrebuildWithStatus[]>([]);
const [isPrebuildPending, setIsPrebuildPending] = useState<boolean>(false);

useEffect(() => {
if (!project) {
Expand All @@ -46,7 +46,8 @@ export default function () {
const registration = getGitpodService().registerClient({
onPrebuildUpdate: (update: PrebuildWithStatus) => {
if (update.info.projectId === project.id) {
setPrebuilds(prev => [update, ...prev.filter(p => p.info.id !== update.info.id)])
setPrebuilds(prev => [update, ...prev.filter(p => p.info.id !== update.info.id)]);
setIsPrebuildPending(false);
}
}
});
Expand Down Expand Up @@ -126,9 +127,23 @@ export default function () {
history.push(`/${!!team ? 't/'+team.slug : 'projects'}/${projectName}/${pb.id}`);
}

const triggerPrebuild = (branchName: string | null) => {
const triggerPrebuild = async (branchName: string | null) => {
if (project) {
getGitpodService().server.triggerPrebuild(project.id, branchName);
setIsPrebuildPending(true);
try {
const result = await Promise.race([
getGitpodService().server.triggerPrebuild(project.id, branchName),
new Promise((_, reject) => setTimeout(() => reject(new Error('Timed out while waiting for new prebuild')), 30000)),
]) as StartPrebuildResult;
console.log('Successfully triggerred!', result);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to self: Maybe I should remove this debug log.

if (result.done) {
// TODO(janx): Visually highlight the prebuild with `p.info.buildWorkspaceId === result.wsid`
setIsPrebuildPending(false);
}
} catch (error) {
console.error('Running prebuild failed!', error);
setIsPrebuildPending(false);
}
}
}

Expand All @@ -150,7 +165,7 @@ export default function () {
<div className="py-3 pl-3">
<DropDown prefix="Prebuild Status: " contextMenuWidth="w-32" entries={statusFilterEntries()} />
</div>
<button disabled={!project} onClick={() => triggerPrebuild(null)} className="ml-2">Trigger Prebuild</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Regarding the removal of the global Trigger Prebuild button mentioned in #5731 (comment) (Cc @svenefftinge), the linked issue (#5374) in the PR description contains some more context behind this:

From #5374 (comment):

In terms of UX, this button could trigger a modal to confirm prebuild on the default branch. Possible improvements could include selecting a different branch, passing an env var for a prebuild, etc.

From #5374 (comment):

We could skip this and remove the button for this iteration if you think it’s redundant. The idea behind this is that for projects that the latest prebuild failed and you’d like to retry or all the prebuilds have been removed after two weeks, someone could trigger a prebuild from the dashboard.

However, if we want to ship this sooner and we have the bandwidth to add a fix here, showing a spinner + disabling the button on click as you mentioned in #5374 (comment) sounds optimal. 🎯

I refrained from suggesting this earlier as there are also a few more places where a loading button component variant could be used for consistency, like the git integration modal, but we can progressively add it there later. ➿

🍊 🍊 🍊 🍊

Adding below some early design drafts how the confirmation modal could look like when triggering a prebuild from the global trigger prebuild button. Let me know if this does not look interesting or useful here. Using a modal here could allow us in the future to provide a way to insert one-time variables for a prebuild which, etc.

Run Prebuild Run Prebuild with Branch Selection
run-1 run-2

🍋 🍋 🍋 🍋

Regarding the replacement of an existing prebuild upon triggering a new prebuild mentioned in #5731 (comment) (Cc @jankeromnes), I'd think that ideally we'd never replace a prebuild but cancel the previous one and creating a new one. Maybe this is currently not technically possible or something we're interested in? 💭

<button disabled={!project || isPrebuildPending} onClick={() => triggerPrebuild(null)} className="ml-2 flex items-center space-x-2"><span>Trigger Prebuild</span>{isPrebuildPending && <img className="h-4 w-4 animate-spin filter brightness-150" src={Spinner} />}</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: What do you think of placing the spinner on the left of the label? Spinner placement is a minor issue and we could go with any approach but I remember discussing something similar with the GitLab team in the past in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/26035#note_297307469. Cross-posting below the relevant comment for visibility within our team:

... regarding spinner placement, although there is not an strict established pattern or design guideline about this, it's mostly related to a) usability (screen reading) and b) LTR (left-to-right) v. RTL (right-to-left) languages where user interfaces should be mirrored to ensure content is easy to understand.

Since we currently only support LTR languages, there's no need to also add a component variant with the spinner on the right side. Once and if there is support for RTL languages we should consider adding a component variant with the spinner placed on the right.

BEFORE AFTER
spinner-before spinner-after

Copy link
Contributor

Choose a reason for hiding this comment

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

issue: Changing the state of this button whenever someone triggers an arbitrary prebuild form the prebuilds list, as mentioned in #5731 (comment), can be confusing to the user. I'd suggest to keep the state of the button intact instead.

</div>
<ItemsList className="mt-2">
<Item header={true} className="grid grid-cols-3">
Expand Down Expand Up @@ -233,7 +248,7 @@ export function prebuildStatusIcon(status: PrebuiltWorkspaceState | undefined) {
}
}

export function PrebuildInstanceStatus(props: { prebuildInstance?: WorkspaceInstance, isDark?: boolean }) {
export function PrebuildInstanceStatus(props: { prebuildInstance?: WorkspaceInstance }) {
let status = <></>;
let details = <></>;
switch (props.prebuildInstance?.status.phase) {
Expand All @@ -244,7 +259,7 @@ export function PrebuildInstanceStatus(props: { prebuildInstance?: WorkspaceInst
<span>PENDING</span>
</div>;
details = <div className="flex space-x-1 items-center text-gray-400">
<img className="h-4 w-4 animate-spin" src={props.isDark ? SpinnerDark : Spinner} />
<img className="h-4 w-4 animate-spin" src={Spinner} />
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: Friendly reminder to come back to this if we ever adjust the spinner color used for light and dark themes. 🌔

<span>Prebuild in progress ...</span>
</div>;
break;
Expand All @@ -259,7 +274,7 @@ export function PrebuildInstanceStatus(props: { prebuildInstance?: WorkspaceInst
<span>RUNNING</span>
</div>;
details = <div className="flex space-x-1 items-center text-gray-400">
<img className="h-4 w-4 animate-spin" src={props.isDark ? SpinnerDark : Spinner} />
<img className="h-4 w-4 animate-spin" src={Spinner} />
<span>Prebuild in progress ...</span>
</div>;
break;
Expand Down