[Create WS] Various fixes #17135
[Create WS] Various fixes #17135
Conversation
- respect referrer context - useWorkspaceContext returns null instead of being disabled when contextUrl is undefined
|
started the job as gitpod-build-se-create-ws-page.1 because the annotations in the pull request description changed |
|
started the job as gitpod-build-se-create-ws-page.2 because the annotations in the pull request description changed |
|
started the job as gitpod-build-se-create-ws-page.3 because the annotations in the pull request description changed |
|
started the job as gitpod-build-se-create-ws-page.4 because the annotations in the pull request description changed |
| @@ -3669,6 +3669,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable { | |||
| async getIDToken(): Promise<void> {} | |||
| public async resolveContext(ctx: TraceContextWithSpan, contextUrl: string): Promise<WorkspaceContext> { | |||
| const user = this.checkAndBlockUser("resolveContext"); | |||
| return this.contextParser.handle(ctx, user, contextUrl); | |||
| const normalizedCtxURL = this.contextParser.normalizeContextURL(contextUrl); | |||
There was a problem hiding this comment.
Is there a time when you wouldn't want to normalize? And if no, can we move the normalization inside the contextParser.handle method?
There was a problem hiding this comment.
There are places where we don't do the normalization, which is when contexts are created based on git hoster events, where we don't need to be fuzzy around the allowed input. It would probably not harm to call the normalization in those cases as well, but I would like to avoid changing the implementation of contextParser with this PR. It's a bit complex and I don't see enough value in the proposed change.
svenefftinge
force-pushed
the
se/create-ws-page
branch
from
2d50a8a to
de331e3
Compare
|
Looking at this now! 👀 |
There was a problem hiding this comment.
Took another look at this and added some UX suggestions for the overall approach and copy.
Thanks for adding this, @svenefftinge. 🌊 🌊
Approving to unblock but holding to let someone take a closer look at the code changes and possibly update the UX.
/hold
| @@ -214,6 +257,24 @@ export function CreateWorkspacePage() { | |||
| {isLoading ? "Loading ..." : isStarting ? "Creating Workspace ..." : "New Workspace"} | |||
| </Button> | |||
| </div> | |||
| {existingWorkspaces.length > 0 && ( | |||
| <div className="w-full flex flex-col justify-end px-6"> | |||
| <p className="mt-6 text-center text-base">Running workspaces on this revision</p> | |||
There was a problem hiding this comment.
question: If it's not too much trouble and we want to add this in the scope of this PR, it could be helpful to be specific about the number of similar workspaces.
// BEFORE
Running workspaces on this branch
// AFTER
3 running workspaces on this branch
| @@ -214,6 +257,24 @@ export function CreateWorkspacePage() { | |||
| {isLoading ? "Loading ..." : isStarting ? "Creating Workspace ..." : "New Workspace"} | |||
| </Button> | |||
| </div> | |||
| {existingWorkspaces.length > 0 && ( | |||
| <div className="w-full flex flex-col justify-end px-6"> | |||
| <p className="mt-6 text-center text-base">Running workspaces on this revision</p> | |||
There was a problem hiding this comment.
question: Definitely out of the scope of this PR, but I wonder if including also stopped workspaces could make sense here. If a user has a stopped workspace on the same branch (revision), does it make sense to also include it here?
| @@ -214,6 +257,24 @@ export function CreateWorkspacePage() { | |||
| {isLoading ? "Loading ..." : isStarting ? "Creating Workspace ..." : "New Workspace"} | |||
| </Button> | |||
| </div> | |||
| {existingWorkspaces.length > 0 && ( | |||
| <div className="w-full flex flex-col justify-end px-6"> | |||
| <p className="mt-6 text-center text-base">Running workspaces on this revision</p> | |||
There was a problem hiding this comment.
praise: I can't skip saying how good this addition generally feels. ✨
| @@ -214,6 +257,24 @@ export function CreateWorkspacePage() { | |||
| {isLoading ? "Loading ..." : isStarting ? "Creating Workspace ..." : "New Workspace"} | |||
| </Button> | |||
| </div> | |||
| {existingWorkspaces.length > 0 && ( | |||
| <div className="w-full flex flex-col justify-end px-6"> | |||
| <p className="mt-6 text-center text-base">Running workspaces on this revision</p> | |||
There was a problem hiding this comment.
question: I couldn't hold from thinking how this list could be better visualized. What do you think of collapsing the list inside a dropdown?
For running workspaces, I guess this won't be usually more than 2 or 3, but there could be edge cases where this could break the UI by overflowing the height of this feature. Additionally, replacing the primary action button with a block button is something that looks needed here.
| BEFORE | AFTER |
|---|---|
 |
 |
| <a | ||
| key={w.workspace.id} | ||
| href={w.latestInstance?.ideUrl || `/start/${w.workspace.id}}`} | ||
| className="rounded-xl group hover:bg-gray-100 dark:hover:bg-gray-800 flex" | ||
| > | ||
| <WorkspaceEntry info={w} shortVersion={true} /> | ||
| </a> |
There was a problem hiding this comment.
suggestion: If we follow the dropdown approach, here's how this could look like:
 |
 |
|
/reviewpad summarize |
|
See this Slack (internal) thread for context. (edited by @axonasif) AI-Generated Summary: This pull request consists of 3 patches:
|
|
/unhold |
roboquat
added
deployed: webapp
Description
Several smaller improvements to the create workspace page
referrerprefix URLs, so that the workspace starts immediately
cc @gtsiolisRelated Issue(s)
Fixes https://linear.app/gitpod/issue/WEB-68/startworkspacewithoptions-invalid-url-error-on-missing-scheme
Fixes https://linear.app/gitpod/issue/WEB-81/context-urls-with-referrer-dont-show-modal
Fixes https://linear.app/gitpod/issue/WEB-73/[new-workspace-page]-dont-start-more-than-one-workspace-with-enter
How to test
Release Notes
Documentation
Build Options:
Run the build with werft instead of GHA
Run Leeway with
--dont-testPublish Options
Installer Options
Add desired feature flags to the end of the line above, space separated
Preview Environment Options:
If enabled this will build
install/previewIf enabled this will create the environment on GCE infra
Valid options are
all,workspace,webapp,ide,jetbrains,vscode,ssh