NotificationsYou must be signed in to change notification settings
Fork928
Star10.1k

chore(site): remove xstate#10659

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

BrunoQuaresma merged 33 commits intomainfrombq/refactor-workspace-xservice

Nov 14, 2023

Merged

chore(site): remove xstate#10659

BrunoQuaresma merged 33 commits intomainfrombq/refactor-workspace-xservice

Nov 14, 2023

Conversation

Copy link

Collaborator

BrunoQuaresma commentedNov 13, 2023•
edited
Loading

Recommendation for the reviewers: Focus on thequeries andWorkspacePage folders.

Close#9943
Close#9598

BrunoQuaresma added26 commits

November 9, 2023 18:20

Remove template deprecation warning

3c99474

Move deployment values fetching

878f995

Remove unused code from template warning

cdf8859

Extract initial data

964c3d2

Move quota fetching to be used close to where it is used

2cd9de1

Fix test on Terminal because of workspace query changes

4fd62bb

Merge branch 'main' ofhttps://github.com/coder/coderinto bq/refacto…

168024d

…r-workspace-xservice

Move can suto start to be on WorkspaceReadyPage

b6ee59a

Extract ssh config

2388cae

Extract watching workspace out of xstate

3d96b94

Remove unused code

b0fba32

Organize code on ready page

9edfd71

Extract change version from xstate

3e1de93

Extract update workspace

ab8977c

Extract delete

cb2c966

Extract activate

6c8ec9f

Remove unecessary require permission

039f4bb

Remove unused RequirePermission usage

987f14f

Extract refresh timeline

13e954f

Remove REFRESH_WORKSPACE

764f047

Extract stop

633c431

Extract start

20e073f

Extract cancellation and remove xservices

bfcc99d

Clean up

cf6896a

Add retry with debug

b6c9469

Remove XState

a4a33f9

BrunoQuaresma requested review fromaslilac,Kira-Pilot andParkreiner

November 13, 2023 19:50

BrunoQuaresma self-assigned this

Nov 13, 2023

Copy link

Member

Parkreiner commentedNov 13, 2023

Started going through this, but I'm not close to done yet. I'm probably just going to spend a few hours going through this first thing tomorrow morning

aslilac reviewed

Nov 13, 2023

View reviewed changes

site/src/pages/WorkspacePage/WorkspacePage.tsxShow resolvedHide resolved

Copy link

Member

aslilac commentedNov 13, 2023

I gave it a relatively thorough scrub and think it looks great!

Parkreiner requested changes

Nov 14, 2023

View reviewed changes

Copy link

Member

Parkreiner left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Echoing what Kayla said – this looks really good, and it's so much easier for me to understand what the retry builds function is doing now. That should help me wrap up my other feature really quickly

I did catch one likely bug with theuseEffect call where it might close out earlier than expected. Beyond that, though, just minor nits/questions

site/src/api/queries/workspaces.ts

		export const workspaceByOwnerAndName = (
		owner: string,
		name: string,
		): QueryOptions<Workspace> => {

Copy link

Member

Choose a reason for hiding this comment

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

Is there a reason why the return type was removed? If nothing else, I feel that some kind of type hint can be worth it , just because it helps with auto-complete

Copy link

CollaboratorAuthor

Choose a reason for hiding this comment

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

It was because when using thequeryKey value from theQueryOptions it was returning a type error because it can be undefined:

Example:

queryClient.setQueryData(workspaceQueryOptions.queryKey,newWorkspaceData,);

Error:

Argument of type 'QueryKey | undefined' is not assignable to parameter of type 'QueryKey'

I could try to better type it but for this case, I just think it is too much. At some point, we may create a utility type to help us with typing the query results.

Copy link

Member

Choose a reason for hiding this comment

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

Yeah, RQ v5's newqueryOptions function might also help with this

site/src/api/queries/workspaces.ts OutdatedShow resolvedHide resolved

site/src/api/queries/workspaces.ts


		export const cancelBuild = (workspace: Workspace) => {
		return {
		mutationFn: () => {

Copy link

Member

ParkreinerNov 14, 2023•
edited
Loading

Choose a reason for hiding this comment

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

Just making sure: does anything need to happen in response to a build being canceled? No query invalidation or cache updates?

Copy link

CollaboratorAuthor

BrunoQuaresmaNov 14, 2023•
edited
Loading

Choose a reason for hiding this comment

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

Good catch! I left this to think in a different moment, and I forgot. For some reason, for this endpoint, we return a defaultResponse object, and in the previous machine, we didn't make anything, but I think the best would be to invalidate the builds query. Should we invalidate any other data?

Copy link

Member

ParkreinerNov 14, 2023

Choose a reason for hiding this comment

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

Yeah, I think that's the most important part. Maybe also the general workspace query, just because it also has a property with theWorkspaceBuilds type?

site/src/pages/WorkspacePage/permissions.ts

		},
		action: "read",
		},
		}) as const;

Copy link

Member

ParkreinerNov 14, 2023

Choose a reason for hiding this comment

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

Do you think it would help to have a separate type defined that lists what each value of the workspace checks object is allowed to be?

It looks like the type is based on theAuthorizationCheck type fromtypesGenerated.ts?

Copy link

CollaboratorAuthor

BrunoQuaresmaNov 14, 2023

Choose a reason for hiding this comment

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

I don't think so 🤔

site/src/pages/WorkspacePage/WorkspacePage.tsx OutdatedShow resolvedHide resolved

site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx

		const buildLogs = useWorkspaceBuildLogs(workspace.latest_build.id);
		const shouldDisplayBuildLogs =
		hasJobError(workspace) \|\|
		["canceling", "deleting", "pending", "starting", "stopping"].includes(
		workspace.latest_build.status,
		);

		// Restart
		const [confirmingRestart, setConfirmingRestart] = useState<{

Copy link

Member

ParkreinerNov 14, 2023

Choose a reason for hiding this comment

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

Wondering if the state type could be defined as a discriminated union, so that you're guaranteed to havebuildParameters be defined wheneveropen is true

Though I guess that would also require updating the type definitions further down the tree

Copy link

CollaboratorAuthor

BrunoQuaresmaNov 14, 2023

Choose a reason for hiding this comment

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

I usually think discriminated unions are always a good idea but for this case, I would just let it as it is since there is not too much value and to avoid more changes.

Copy link

Member

ParkreinerNov 14, 2023

Choose a reason for hiding this comment

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

Yeah, that's fair

site/src/pages/WorkspacePage/WorkspaceReadyPage.tsxShow resolvedHide resolved

BrunoQuaresma added4 commits

November 14, 2023 15:51

Merge branch 'main' ofhttps://github.com/coder/coderinto bq/refacto…

e72abf5

…r-workspace-xservice

Clean up event source

75d39a2

Rename workspace mutations

c3de809

Invalidate workspace builds data on cancel

477b928

Kira-Pilot reviewed

Nov 14, 2023

View reviewed changes

site/src/api/queries/workspaces.ts OutdatedShow resolvedHide resolved

BrunoQuaresma added2 commits

November 14, 2023 16:42

use useEffectEvent to avoid unecessary updates

d189c20

Merge branch 'main' ofhttps://github.com/coder/coderinto bq/refacto…

49b95a2

…r-workspace-xservice

BrunoQuaresma requested a review fromParkreiner

November 14, 2023 16:50

Copy link

Member

Kira-Pilot commentedNov 14, 2023•
edited
Loading

I notice that if I stop/start a build and then cancel the action halfway through, I'm getting an API error every time:

This isn't happening onmain so I'm just wondering if maybe we're not timing something properly on the FE - do we have to wait for the build to finish before we cancel it, perhaps?

Fix cancel action

6cc257a

Copy link

CollaboratorAuthor

BrunoQuaresma commentedNov 14, 2023

@Kira-Pilot good catch! I was passing the workspace id instead of the build id 🤦 I also noted here to create a test for it.

Parkreiner approved these changes

Nov 14, 2023

View reviewed changes

Copy link

Member

Parkreiner left a comment•
edited
Loading

Choose a reason for hiding this comment

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

If Kira's issues are resolved, no other concerns jump out at me. Just had a comment about one other type of query invalidation that could be added

site/src/api/queries/workspaces.ts


		export const cancelBuild = (workspace: Workspace) => {
		return {
		mutationFn: () => {

Copy link

Member

ParkreinerNov 14, 2023

Choose a reason for hiding this comment

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

Yeah, I think that's the most important part. Maybe also the general workspace query, just because it also has a property with theWorkspaceBuilds type?

site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx

		const buildLogs = useWorkspaceBuildLogs(workspace.latest_build.id);
		const shouldDisplayBuildLogs =
		hasJobError(workspace) \|\|
		["canceling", "deleting", "pending", "starting", "stopping"].includes(
		workspace.latest_build.status,
		);

		// Restart
		const [confirmingRestart, setConfirmingRestart] = useState<{

Copy link

Member

ParkreinerNov 14, 2023

Choose a reason for hiding this comment

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

Yeah, that's fair

Copy link

CollaboratorAuthor

BrunoQuaresma commentedNov 14, 2023•
edited
Loading

Yeah, I think that's the most important part. Maybe also the general workspace query, just because it also has a property with the WorkspaceBuilds type?

The workspace is going to be automatically updated because of the SSE connection so I would avoid having two events updating the same source.

Kira-Pilot approved these changes

Nov 14, 2023

View reviewed changes

Copy link

Member

Kira-Pilot left a comment

Choose a reason for hiding this comment

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

This looks great, Bruno! Thanks so much for taking it on.

BrunoQuaresmaenabled auto-merge (squash)

November 14, 2023 18:33

BrunoQuaresma merged commit90b6e86 intomain

Nov 14, 2023

BrunoQuaresma deleted the bq/refactor-workspace-xservice branch

November 14, 2023 18:34

ericpaulsen pushed a commit to lbi22/coder that referenced this pull request

Nov 27, 2023

chore(site): remove xstate (coder#10659)

6ae0975

Labels

None yet

4 participants

Movatterモバイル変換

chore(site): remove xstate#10659

chore(site): remove xstate#10659

Uh oh!

Conversation

BrunoQuaresma commentedNov 13, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Parkreiner commentedNov 13, 2023

Uh oh!

Uh oh!

aslilac commentedNov 13, 2023

Uh oh!

Parkreiner left a comment• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ParkreinerNov 14, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrunoQuaresmaNov 14, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Kira-Pilot commentedNov 14, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

BrunoQuaresma commentedNov 14, 2023

Uh oh!

Parkreiner left a comment• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrunoQuaresma commentedNov 14, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Kira-Pilot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BrunoQuaresma commentedNov 13, 2023•
edited
Loading

Parkreiner left a comment•
edited
Loading

ParkreinerNov 14, 2023•
edited
Loading

BrunoQuaresmaNov 14, 2023•
edited
Loading

Kira-Pilot commentedNov 14, 2023•
edited
Loading

Parkreiner left a comment•
edited
Loading

BrunoQuaresma commentedNov 14, 2023•
edited
Loading