Skip to content
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

[dashboard] show repo name when workspace creation fails #7302

Merged
merged 3 commits into from
Jan 24, 2022

Conversation

trumbitta
Copy link
Contributor

Description

When the creation of a workspace fails because the repo wasn't found, also display the repository name near the error messages so that in the scenario described by #7191 it becomes easier to understand what's actually going on.

Related Issue(s)

Fixes #7191

How to test

image

Release Notes

[dashboard] Error messages on workspace creation when the repository is not found, will now also display the name of the repository

Documentation

@iQQBot
Copy link
Contributor

iQQBot commented Dec 19, 2021

/werft run

👍 started the job as gitpod-build-trumbitta-display-repo-name-7191-fork.0

@iQQBot
Copy link
Contributor

iQQBot commented Dec 19, 2021

image

@iQQBot
Copy link
Contributor

iQQBot commented Dec 19, 2021

Sometimes, the message returned from the server already carries the repo name
My suggestion is to adjust the server so that all messages returned contain the repo name, which is better than adjusting the dashboard

@trumbitta
Copy link
Contributor Author

trumbitta commented Dec 19, 2021

Great catch!

I'd rather have the message never include the repo name though: I find it to be clearer in the new position added by this PR, and it also makes it easier to use a <code> HTML tag which is the semantically appropriate way to include a repo name in the document 🤔

@trumbitta
Copy link
Contributor Author

Sometimes, the message returned from the server already carries the repo name My suggestion is to adjust the server so that all messages returned contain the repo name, which is better than adjusting the dashboard

Wait a minute, hold your horses: it seems we both have been misguided and the actual user-facing messages are in the frontend component (as they should!).

I'll work on an update asap!

@trumbitta trumbitta force-pushed the trumbitta/display-repo-name-7191 branch from 9958478 to 4832121 Compare December 21, 2021 12:34
@trumbitta
Copy link
Contributor Author

trumbitta commented Dec 21, 2021

@iQQBot ok, done. I also added a commit with a quick semantic improvement for the repo owner in the messages 🍻

@iQQBot
Copy link
Contributor

iQQBot commented Dec 21, 2021

/werft run

👍 started the job as gitpod-build-trumbitta-display-repo-name-7191-fork.1

@iQQBot
Copy link
Contributor

iQQBot commented Dec 21, 2021

@gtsiolis Is UI looks good?

@stale
Copy link

stale bot commented Jan 1, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the meta: stale This issue/PR is stale and will be closed soon label Jan 1, 2022
@trumbitta
Copy link
Contributor Author

trumbitta commented Jan 1, 2022

Please good bot, keep it open 🙏

@stale stale bot removed the meta: stale This issue/PR is stale and will be closed soon label Jan 1, 2022
@JanKoehnlein JanKoehnlein requested a review from gtsiolis January 5, 2022 13:26
@gtsiolis
Copy link
Contributor

gtsiolis commented Jan 5, 2022

/werft run

👍 started the job as gitpod-build-trumbitta-display-repo-name-7191-fork.2

Copy link
Member

@Siddhant-K-code Siddhant-K-code left a comment

Choose a reason for hiding this comment

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

I think we should return the Repo. Name too.

@@ -259,15 +259,15 @@ function RepositoryNotFoundView(p: { error: StartWorkspaceError }) {

if (!userScopes.includes(missingScope)) {
setStatusMessage(<div className="mt-2 flex flex-col space-y-8">
<p className="text-base text-gray-400 w-96">The repository '{`${repoFullName}`}' may be private. Please authorize Gitpod to access to private repositories.</p>
<p className="text-base text-gray-400 w-96">The repository may be private. Please authorize Gitpod to access to private repositories.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<p className="text-base text-gray-400 w-96">The repository may be private. Please authorize Gitpod to access to private repositories.</p>
<p className="text-base text-gray-400 w-96">The repository <code>${repoName}</code> may be private. Please authorize Gitpod to access to private repositories.</p>

Copy link
Contributor Author

@trumbitta trumbitta Jan 5, 2022

Choose a reason for hiding this comment

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

Care to elaborate on why?

I reckon that keeping it inside the error message makes it pass as "unimportant" and makes it also easily misread.
In the new proposed position the repo name is front and center, bigger, red: meaning that next time, the next user won't likely have the same hiccup @ghuntley lamented about in #7191 😄

Copy link
Member

Choose a reason for hiding this comment

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

Just a more informative way to display to the users, maybe!?

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 forgot to mention that keeping the repo name also in the error message looked bad both to me and @iQQBot . See #7302 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @Siddhant-K-code @trumbitta! Agree, keeping the repository out of the error message is better as this information is now anchored in a single place that also helps memory muscle. 💪

@gtsiolis
Copy link
Contributor

gtsiolis commented Jan 10, 2022

Looking at this now! 👀

Copy link
Contributor

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

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

Thanks @trumbitta for contributing, @Siddhant-K-code for chiming in, and @iQQBot for doing a first review round! 🏀

@trumbitta Changes here definitely improve overall user experience of this page. 🔮

Left some minor comments below. Let me know what you think but we could also leave these out of the scope of this PR. Your call.

Approving to unblock merging but holding in case we could tackle some minor UX issues as mentioned below.

/hold

@@ -259,15 +259,15 @@ function RepositoryNotFoundView(p: { error: StartWorkspaceError }) {

if (!userScopes.includes(missingScope)) {
setStatusMessage(<div className="mt-2 flex flex-col space-y-8">
<p className="text-base text-gray-400 w-96">The repository '{`${repoFullName}`}' may be private. Please authorize Gitpod to access to private repositories.</p>
<p className="text-base text-gray-400 w-96">The repository may be private. Please authorize Gitpod to access to private repositories.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @Siddhant-K-code @trumbitta! Agree, keeping the repository out of the error message is better as this information is now anchored in a single place that also helps memory muscle. 💪

return (
<StartPage phase={StartPhase.Checking} error={p.error}>
<p className="text-base text-gitpod-red mt-2">
<code>{repoFullName}</code>
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 using the preformatted CodeText component used elsewhere to format this information, instead of using <code> tag?

For example, see:

<p className="text-base text-gray-500 pb-4 max-w-2xl">The following information will be used to set up Git configuration. You can override Git author name and email per project by using the default environment variables <CodeText>GIT_AUTHOR_NAME</CodeText>, <CodeText>GIT_COMMITTER_NAME</CodeText>, <CodeText>GIT_AUTHOR_EMAIL</CodeText> and <CodeText>GIT_COMMITTER_EMAIL</CodeText>.</p>

BEFORE AFTER
Screenshot 2022-01-10 at 12 53 55 PM (2) Screenshot 2022-01-10 at 12 54 23 PM (2)

Copy link
Contributor Author

@trumbitta trumbitta Jan 10, 2022

Choose a reason for hiding this comment

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

Thanks! I didn't know about CodeText, and it looked promising at first... but it's a span whereas I would rather use a code for this.

What's the intended use of CodeText? Why would I want to have a styled span that looks like a styled code instead of using a styled code?
For example, those variable names in Account.tsx should be marked up with a code.

So now I need advice on how to handle this in the best interest of the project 😊:

  • Use CodeText as-is (🙈 I'd rather not contribute more non-semantic markup)
  • Leave code here but copy-paste the style from CodeText (🤕 repetition over encapsulation)
  • Modify CodeText to be a code instead of a span (🤔 this is my choice, but are there any cases in the codebase where a span is actually the right element?)
  • Make CodeText into a polymorphic component via a classic as property, defaulting to span for retro-compatibility (🖌 might be too much, given it's unlikely anyone would ever want CodeText to render as a section, or a dl, or anything other than a span or a code.

Copy link
Contributor

Choose a reason for hiding this comment

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

🍊 Answering some questions:

Why would I want to have a styled span that looks like a styled code instead of using a styled code?

@trumbitta Using <code> tag to render such elements sounds right, especially since this is broadly related to rendering markdown elements like code blocks and inline code. The idea behind CodeText component was to wrap such elements and provide proper styling when rendering instances across the dashboard.

For example, those variable names in Account.tsx should be marked up with a code.

@trumbitta You're right and this can also help with accessibility and component consistency. I'm also open for discussing a different design between inline code like the variables in /account and the repository full name in the error pages here. 💯

🍋 Some background context:

FWIW, for historical reasons, the CodeText component was introduced as a side fix during a larger feature implementation in #4607, to replace the already existing span-based elements from the initial dashboard redesign. To help cut some corners and unblock shipping that larger feature, we had to limit scope creep by limiting improvements related to these code elements to just componetizing the already existing elements. 🔖

FYI, we're approaching the development of the dashboard using an MVC (Minimum Viable Change) approach and we iteratively improve existing or add new functionalities. ➿

🥝 Proposal:

  1. Taking all the above into account, in the spirit of MVC and iterating step-by-step as product needs grow, I'd suggest to keep the CodeText component as is for now. If you could use CodeText for replacing the repository full name below that would be great. Otherwise, feel free to leave this as is.
  2. Next steps could include improving this CodeText component by using the <code> tag under the hood instead of a <span>, while checking if this renders ok across the dashboard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

💯 Makes sense.
Thanks for the bit of history and context explanation: now I feel more aligned with the approach and guiding principles 🎛️ . It will be easier to contribute and think strategically 🧐 !

Balancing MVC (first time I see this acronym with a glorious past used with this meaning 😸 ) with my old core principle of continuous refactoring (here's a fun take by Kent Beck: https://docs.google.com/document/d/e/2PACX-1vRS232prL-Bq44hmmdzJTHkCz-nYBlQVPAtemOmVfDP1rbiD777jQAaUpIeF2KCdSSEJMEwHTeo9QdE/pub) promises to be a refreshing and positive challenge 🙇 so thank you for that 🙏

🪡 I'll update the bits you pointed out to use CodeText, and I will open an issue for improving it with the <code> tag.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks a lot again @gtsiolis for the detailed explanation! ❤️

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @Siddhant-K-code @trumbitta! 🙏 💯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gtsiolis done!

@@ -284,22 +284,27 @@ function RepositoryNotFoundView(p: { error: StartWorkspaceError }) {

if (!updatedRecently) {
setStatusMessage(<div className="mt-2 flex flex-col space-y-8">
<p className="text-base text-gray-400 w-96">Permission to access private repositories has been granted. If you are a member of '{owner}', please try to request access for Gitpod.</p>
<p className="text-base text-gray-400 w-96">Permission to access private repositories has been granted. If you are a member of <code>{owner}</code>, please try to request access for Gitpod.</p>
<a className="mx-auto" href={authorizeURL}><button className="secondary">Request Access for Gitpod</button></a>
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick(non-blocking): No need to keep the secondary button class here as this is the only primary action forward.

Suggested change
<a className="mx-auto" href={authorizeURL}><button className="secondary">Request Access for Gitpod</button></a>
<a className="mx-auto" href={authorizeURL}><button>Request Access for Gitpod</button></a>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, pushing in a minute 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Resolved in 653f02e.

<a className="mx-auto" href={authorizeURL}><button className="secondary">Request Access for Gitpod</button></a>
</div>);
return;
}

setStatusMessage(<div className="mt-2 flex flex-col space-y-8">
<p className="text-base text-gray-400 w-96">Your access token was updated recently. Please try again if the repository exists and Gitpod was approved for '{owner}'.</p>
<p className="text-base text-gray-400 w-96">Your access token was updated recently. Please try again if the repository exists and Gitpod was approved for <code>{owner}</code>.</p>
<a className="mx-auto" href={authorizeURL}><button className="secondary">Try Again</button></a>
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick(non-blocking): No need to keep the secondary button class here as this is the only primary action forward.

Suggested change
<a className="mx-auto" href={authorizeURL}><button className="secondary">Try Again</button></a>
<a className="mx-auto" href={authorizeURL}><button>Try Again</button></a>

Copy link
Contributor

Choose a reason for hiding this comment

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

Resolved in 653f02e.

@@ -259,15 +259,15 @@ function RepositoryNotFoundView(p: { error: StartWorkspaceError }) {

if (!userScopes.includes(missingScope)) {
setStatusMessage(<div className="mt-2 flex flex-col space-y-8">
<p className="text-base text-gray-400 w-96">The repository '{`${repoFullName}`}' may be private. Please authorize Gitpod to access to private repositories.</p>
<p className="text-base text-gray-400 w-96">The repository may be private. Please authorize Gitpod to access to private repositories.</p>
<a className="mx-auto" href={authorizeURL}><button className="secondary">Grant Access</button></a>
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick(non-blocking): No need to keep the secondary button class here as this is the only primary action forward.

Suggested change
<a className="mx-auto" href={authorizeURL}><button className="secondary">Grant Access</button></a>
<a className="mx-auto" href={authorizeURL}><button>Grant Access</button></a>

Copy link
Contributor

Choose a reason for hiding this comment

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

Resolved in 653f02e.

<a className="mx-auto" href={authorizeURL}><button className="secondary">Request Access for Gitpod</button></a>
</div>);
return;
}

setStatusMessage(<div className="mt-2 flex flex-col space-y-8">
<p className="text-base text-gray-400 w-96">Your access token was updated recently. Please try again if the repository exists and Gitpod was approved for '{owner}'.</p>
<p className="text-base text-gray-400 w-96">Your access token was updated recently. Please try again if the repository exists and Gitpod was approved for <code>{owner}</code>.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: We could probably use the same CodeText component here as mentioned below. 💭

@@ -284,22 +284,27 @@ function RepositoryNotFoundView(p: { error: StartWorkspaceError }) {

if (!updatedRecently) {
setStatusMessage(<div className="mt-2 flex flex-col space-y-8">
<p className="text-base text-gray-400 w-96">Permission to access private repositories has been granted. If you are a member of '{owner}', please try to request access for Gitpod.</p>
<p className="text-base text-gray-400 w-96">Permission to access private repositories has been granted. If you are a member of <code>{owner}</code>, please try to request access for Gitpod.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: We could probably use the same CodeText component here as mentioned below. 💭

@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: 0b9c1446fad28347ffbf946cf7a256a47755a85a

@Siddhant-K-code
Copy link
Member

Siddhant-K-code commented Jan 10, 2022

Literally one of the best & informative code review by @gtsiolis

@gtsiolis

This comment has been minimized.

@gtsiolis
Copy link
Contributor

gtsiolis commented Jan 10, 2022

/werft run

👍 started the job as gitpod-build-trumbitta-display-repo-name-7191-fork.3

@gtsiolis
Copy link
Contributor

gtsiolis commented Jan 12, 2022

/werft run

👍 started the job as gitpod-build-trumbitta-display-repo-name-7191-fork.4

@gtsiolis
Copy link
Contributor

gtsiolis commented Jan 12, 2022

UX looks great, @trumbitta! 🔮

@jankeromnes could you take another look when you're free since you've recently edited these files?

@gtsiolis gtsiolis requested a review from jankeromnes January 12, 2022 19:32
@stale
Copy link

stale bot commented Jan 22, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the meta: stale This issue/PR is stale and will be closed soon label Jan 22, 2022
@trumbitta
Copy link
Contributor Author

oh come on stale bot, you old rascal!

@stale stale bot removed the meta: stale This issue/PR is stale and will be closed soon label Jan 22, 2022
@jankeromnes
Copy link
Contributor

jankeromnes commented Jan 24, 2022

Whoops, looks like I've overseen this review request. Will take a look now! Many thanks 🙏

Copy link
Contributor

@jankeromnes jankeromnes left a comment

Choose a reason for hiding this comment

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

Also looks good to me! 🚀

Many thanks @trumbitta for this improvement; @gtsiolis for the awesome reviews; and @iQQBot and @Siddhant-K-code for helping out! 🙏

Left a few thoughts in-line, but they're not blocking so removing the hold.

Also, the preview environment got cleaned up so I couldn't test this, but since @gtsiolis already approved the UX I don't need to. 🚢

Finally, I feel like adding the repository name inside the top error message would be even better (i.e. Repository gitpod-io/gitpoddenz not found. as opposed to Repository not found.<br/>gitpod-io/gitpoddenz) but looking at the code this looks trickier than anticipated, and the current fix looks good to me too.

/unhold

@@ -17,6 +17,7 @@ import { openAuthorizeWindow } from "../provider-utils";
import { SelectAccountPayload } from "@gitpod/gitpod-protocol/lib/auth";
import { SelectAccountModal } from "../settings/SelectAccountModal";
import { watchHeadlessLogs } from "../components/PrebuildLogs";
import CodeText from "../components/CodeText";
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, I had no idea we had this component. 😅 I agree with @trumbitta that a <code> element would be more semantic than a <span>.

Also, creating single-line React components is not the best way to share common style (because it adds unnecessary code and complexity, and it's too easy to forget about using the component where relevant). I think we should remove the CodeText component again, and instead apply our common CSS rules to the code element inside index.css.

However, this is off-topic here and can definitely be a follow-up issue / Pull Request. @trumbitta would you be interested in filing an issue for this? 🙏

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 had already created one :) might be worth it to chime in and add your perspective? #7555

<p className="text-base text-gray-400 w-96">The repository '{`${repoFullName}`}' may be private. Please authorize Gitpod to access to private repositories.</p>
<a className="mx-auto" href={authorizeURL}><button className="secondary">Grant Access</button></a>
<p className="text-base text-gray-400 w-96">The repository may be private. Please authorize Gitpod to access to private repositories.</p>
<a className="mx-auto" href={authorizeURL}><button>Grant Access</button></a>
Copy link
Contributor

@jankeromnes jankeromnes Jan 24, 2022

Choose a reason for hiding this comment

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

Praise: Removing the secondary class from these buttons is such an awesome drive-by fix. 💯 Many thanks @trumbitta and @gtsiolis. ❤️

@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: 477dc1647bc12f1407f5fdef8c227096f280f7d8

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gtsiolis, jankeromnes

Associated issue: #7191

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@roboquat roboquat merged commit 43198ce into gitpod-io:main Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display name of the repository on the "Repository not found" error screen
6 participants