Skip to content

fix: create and read workspace page #1294

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

Jump to bottom
Merged
merged 13 commits into from
May 6, 2022
Merged

fix: create and read workspace page #1294

merged 13 commits into from
May 6, 2022

Conversation

presleyp
Copy link
Contributor

@presleyp presleyp commented May 4, 2022 •
edited
Loading

This lays groundwork for #1032

  • rename an api call to make room for another one
  • swap swr for xservice
  • add test
  • edit the paths used to read a template and create a workspace because the API has changed since that code was written and they were broken - I did this in the fastest way possible to unblock myself without bloating this PR; I'll change the create workspace flow to use XState when I work on workspace CRUD
  • edit renderWithAuth to work with parameterized routes

@presleyp presleyp self-assigned this May 4, 2022
@codecov
Copy link

codecov bot commented May 4, 2022 •
edited
Loading

Codecov Report

Merging #1294 (78f1708) into main (f911c8a) will increase coverage by 0.34%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##             main    #1294      +/-   ##
==========================================
+ Coverage   66.08%   66.43%   +0.34%     
==========================================
  Files         281      282       +1     
  Lines       18424    18466      +42     
  Branches      220      231      +11     
==========================================
+ Hits        12176    12268      +92     
+ Misses       4984     4940      -44     
+ Partials     1264     1258       -6
Flag Coverage Δ
unittest-go-macos-latest 53.63% <ø> (+0.14%) ⬆️
unittest-go-postgres- 65.00% <ø> (+0.06%) ⬆️
unittest-go-ubuntu-latest 55.98% <ø> (+0.01%) ⬆️
unittest-go-windows-2022 51.88% <ø> (-0.02%) ⬇️
unittest-js 73.14% <71.42%> (+1.53%) ⬆️
Impacted Files Coverage Δ
site/src/AppRouter.tsx 0.00% <ø> (ø)
...anizationPage/TemplatePage/CreateWorkspacePage.tsx 0.00% <0.00%> (ø)
...ges/OrganizationPage/TemplatePage/TemplatePage.tsx 0.00% <0.00%> (ø)
site/src/xServices/workspace/workspaceXService.ts 59.09% <59.09%> (ø)
site/src/api/index.ts 71.83% <90.90%> (+4.08%) ⬆️
site/src/pages/WorkspacePage/WorkspacePage.tsx 91.66% <91.66%> (ø)
site/src/forms/CreateWorkspaceForm.tsx 91.30% <100.00%> (+1.83%) ⬆️
site/src/testHelpers/index.tsx 100.00% <100.00%> (+5.55%) ⬆️
site/src/xServices/StateContext.tsx 92.85% <100.00%> (+0.54%) ⬆️
site/src/xServices/terminal/terminalXService.ts 68.42% <100.00%> (ø)
... and 11 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 f911c8a...78f1708. Read the comment docs.

@presleyp presleyp marked this pull request as ready for review May 5, 2022 04:40
@presleyp presleyp requested a review from a team as a code owner May 5, 2022 04:40
kylecarbs
kylecarbs approved these changes May 5, 2022
View reviewed changes
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.

LGTM!

site/src/forms/CreateWorkspaceForm.tsx
@@ -15,13 +15,19 @@ export interface CreateWorkspaceForm {
template: Template
onSubmit: (request: CreateWorkspaceRequest) => Promise<Workspace>
onCancel: () => void
organization_id: string
Copy link
Member

Choose a reason for hiding this comment

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

Should this be organizationID to match other camelCase fields in this type?

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 left it snakecased because it gets piped directly out of and into api calls, but I agree the inconsistency isn't good. I guess I'll ask about this at FE V, we have some other spots like this.

site/src/pages/WorkspacePage/WorkspacePage.tsx
const workspaceId = firstOrItem(workspaceQueryParam, null)

const xServices = useContext(XServiceContext)
const [workspaceState, workspaceSend] = useActor(xServices.workspaceXService)
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, this uses a global XState service. Would useMachine be more appropriate here since the machine should only be active for the lifecycle of this page?

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 was thinking it would be active for longer but maybe that's not actually desired since it's for a specific workspace. I can make it local for now and useMachine and we can hoist it later if needed.

Copy link
Member

Choose a reason for hiding this comment

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

Neato neato

Copy link
Contributor Author

@presleyp presleyp May 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@kylecarbs Actually I think we'll want it to outlive the component because we're using separate pages for things like the create/edit form and the parts of the timeline. Does that sound right?

@presleyp presleyp changed the title Workspace api/presleyp fix: create and read workspace page May 5, 2022
code-asher
code-asher approved these changes May 5, 2022
View reviewed changes
Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

Looks great!

I wonder if we could tweak the workspace endpoint in the future to include more info so we can avoid three calls on the FE but I might be overthinking it 😂

@presleyp presleyp merged commit a2be7c0 into main May 6, 2022
@presleyp presleyp deleted the workspace-api/presleyp branch May 6, 2022 14:02
@misskniss misskniss added this to the V2 Beta milestone May 15, 2022
kylecarbs pushed a commit that referenced this pull request Jun 10, 2022
@presleyp @kylecarbs
fix: create and read workspace page (#1294)
67a0c74 
* Change name of existing workspace call

* Add new api call (has handler already)

* WorkspacesPage -> WorkspacePage

* starting to replace swr

* Add other api calls

* Fix api call

* Replace swr with xstate

* Format

* Test - wip

* Fix route in template page

* Fix endpoint in create workspace

* Fix tests

* Lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kylecarbs kylecarbs kylecarbs approved these changes

@code-asher code-asher code-asher approved these changes

Assignees

@presleyp presleyp

Labels
None yet
Projects
None yet
Milestone
V2 Beta
Development

Successfully merging this pull request may close these issues.
4 participants
@presleyp @kylecarbs @code-asher @misskniss