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

fix: Update routes for project page, workspace creation page, and workspace page #415

Merged
merged 5 commits into from
Mar 9, 2022

Conversation

bryphe-coder
Copy link
Contributor

@bryphe-coder bryphe-coder commented Mar 8, 2022

Some API routes were updated in #401, which impacted the UX - the flow is currently broken when trying to navigate to a project:

2022-03-08 15 30 59

This fixes all the routes so that the complete project -> create workspace -> workspace page flow works:

2022-03-08 16 18 57

Because this had to touch a bunch of UI routes, I also opportunistically fixed #380 as part of this change.

TODO:

  • Add gifs for each of the flows
  • Filter projects correctly for the projects page
  • Restore organization name to projects paths
  • Fix project name on workspaces page
  • Fix link back to project on workspaces page

@codecov
Copy link

codecov bot commented Mar 8, 2022

Codecov Report

Merging #415 (c953482) into main (ac387a1) will decrease coverage by 0.33%.
The diff coverage is 4.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #415      +/-   ##
==========================================
- Coverage   68.44%   68.11%   -0.34%     
==========================================
  Files         155      160       +5     
  Lines        9052     9256     +204     
  Branches       73       76       +3     
==========================================
+ Hits         6196     6305     +109     
- Misses       2254     2327      +73     
- Partials      602      624      +22     
Flag Coverage Δ
unittest-go-macos-latest 62.74% <ø> (-0.05%) ⬇️
unittest-go-ubuntu-latest 67.42% <ø> (-0.22%) ⬇️
unittest-go-windows-2022 62.14% <ø> (?)
unittest-js 64.16% <4.47%> (-1.87%) ⬇️
Impacted Files Coverage Δ
...pages/projects/[organization]/[project]/create.tsx 0.00% <0.00%> (ø)
.../pages/projects/[organization]/[project]/index.tsx 0.00% <0.00%> (ø)
site/pages/workspaces/[workspace].tsx 0.00% <0.00%> (ø)
site/util/index.ts 0.00% <0.00%> (ø)
site/util/swr.ts 0.00% <0.00%> (ø)
site/components/Workspace/Workspace.tsx 100.00% <100.00%> (ø)
peer/conn.go 79.02% <0.00%> (-2.56%) ⬇️
provisionerd/provisionerd.go 78.49% <0.00%> (-1.11%) ⬇️
pty/pty_windows.go 68.25% <0.00%> (ø)
pty/start_windows.go 60.86% <0.00%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac387a1...c953482. Read the comment docs.

@bryphe-coder bryphe-coder marked this pull request as ready for review March 9, 2022 00:22
Copy link
Member

@kylecarbs kylecarbs left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I'd appreciate @presleyp and @vapurrmaid thoughts here too!

@@ -8,26 +8,36 @@ import { useUser } from "../../../../contexts/UserContext"
import { ErrorSummary } from "../../../../components/ErrorSummary"
import { FullScreenLoader } from "../../../../components/Loader/FullScreenLoader"
import { CreateWorkspaceForm } from "../../../../forms/CreateWorkspaceForm"
import { unsafeSWRArgument } from "../../../../util"
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious what this is, but I'm sure you have a good reason for using it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question @presleyp ! I'm open to ideas for better ways to handle this (and luckily, it's at least temporary - if we switch to XState and migrate from SWR, this utility isn't needed).

With SWR - when one call relies on another call - SWR expects the function to 'throw' until the data is available. So I ended up with a lot of casts to any + suppressions:

  const { data: project, error: projectError } = useSWR<API.Project, Error>(() => {
    // Using `any` below so that, if the project isn't available, an exception is thrown - causing
    // SWR to retry:
    // eslint-disable-line no-any
    return `/api/v2/organizations/${(project as any).id}/projects/${projectName}`
  })

I ended up with a lot of these comments + any casts, so I figured if I extracted it out to a helper, it'd be a little clearer:

  const { data: project, error: projectError } = useSWR<API.Project, Error>(() => {
    return `/api/v2/organizations/${unsafeSWRArgument(project).id}/projects/${projectName}`
  })

I'm happy to switch back to the more verbose comment + eslint disable + cast, or rename unsafeSWRArgument to something more understandable. (Implementation is here if it helps) IMO the most important criteria is that it is readable / understandable for you and @vapurrmaid

And luckily, seems like this will be only temporary - if we remove SWR and use XState, this will no longer be an issue and this can be removed

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see where you defined! (Thought it was an SWR thing at first)

@bryphe-coder
Copy link
Contributor Author

Thanks @presleyp and @kylecarbs for the review!

@bryphe-coder bryphe-coder merged commit 9f19041 into main Mar 9, 2022
@bryphe-coder bryphe-coder deleted the bryphe/fix/updated-ux-routes branch March 9, 2022 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API: Users, Projects, and Workspaces middleware should support UUID and friendly names
3 participants