From 5f516ed135118ee721021c5722b62f3ea5c8cd2e Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Tue, 6 May 2025 16:00:16 +0200 Subject: [PATCH 1/5] feat: improve coder connect tunnel handling on reconnect (#17598) Closes https://github.com/coder/internal/issues/563 The [Coder Connect tunnel](https://github.com/coder/coder/blob/main/vpn/tunnel.go) receives workspace state from the Coder server over a [dRPC stream.](https://github.com/coder/coder/blob/114ba4593b2a82dfd41cdcb7fd6eb70d866e7b86/tailnet/controllers.go#L1029) When first connecting to this stream, the current state of the user's workspaces is received, with subsequent messages being diffs on top of that state. However, if the client disconnects from this stream, such as when the user's device is suspended, and then reconnects later, no mechanism exists for the tunnel to differentiate that message containing the entire initial state from another diff, and so that state is incorrectly applied as a diff. In practice: - Tunnel connects, receives a workspace update containing all the existing workspaces & agents. - Tunnel loses connection, but isn't completely stopped. - All the user's workspaces are restarted, producing a new set of agents. - Tunnel regains connection, and receives a workspace update containing all the existing workspaces & agents. - This initial update is incorrectly applied as a diff, with the Tunnel's state containing both the old & new agents. This PR introduces a solution in which tunnelUpdater, when created, sends a FreshState flag with the WorkspaceUpdate type. This flag is handled in the vpn tunnel in the following fashion: - Preserve existing Agents - Remove current Agents in the tunnel that are not present in the WorkspaceUpdate - Remove unreferenced Workspaces --- tailnet/controllers.go | 32 ++- tailnet/controllers_test.go | 9 +- vpn/tunnel.go | 77 +++++-- vpn/tunnel_internal_test.go | 396 ++++++++++++++++++++++++++++++++++++ 4 files changed, 498 insertions(+), 16 deletions(-) diff --git a/tailnet/controllers.go b/tailnet/controllers.go index 2328e19640a4d..b7d4e246a4bee 100644 --- a/tailnet/controllers.go +++ b/tailnet/controllers.go @@ -897,6 +897,21 @@ type Workspace struct { agents map[uuid.UUID]*Agent } +func (w *Workspace) Clone() Workspace { + agents := make(map[uuid.UUID]*Agent, len(w.agents)) + for k, v := range w.agents { + clone := v.Clone() + agents[k] = &clone + } + return Workspace{ + ID: w.ID, + Name: w.Name, + Status: w.Status, + ownerUsername: w.ownerUsername, + agents: agents, + } +} + type DNSNameOptions struct { Suffix string } @@ -1049,6 +1064,7 @@ func (t *tunnelUpdater) recvLoop() { t.logger.Debug(context.Background(), "tunnel updater recvLoop started") defer t.logger.Debug(context.Background(), "tunnel updater recvLoop done") defer close(t.recvLoopDone) + updateKind := Snapshot for { update, err := t.client.Recv() if err != nil { @@ -1061,8 +1077,10 @@ func (t *tunnelUpdater) recvLoop() { } t.logger.Debug(context.Background(), "got workspace update", slog.F("workspace_update", update), + slog.F("update_kind", updateKind), ) - err = t.handleUpdate(update) + err = t.handleUpdate(update, updateKind) + updateKind = Diff if err != nil { t.logger.Critical(context.Background(), "failed to handle workspace Update", slog.Error(err)) cErr := t.client.Close() @@ -1083,14 +1101,23 @@ type WorkspaceUpdate struct { UpsertedAgents []*Agent DeletedWorkspaces []*Workspace DeletedAgents []*Agent + Kind UpdateKind } +type UpdateKind int + +const ( + Diff UpdateKind = iota + Snapshot +) + func (w *WorkspaceUpdate) Clone() WorkspaceUpdate { clone := WorkspaceUpdate{ UpsertedWorkspaces: make([]*Workspace, len(w.UpsertedWorkspaces)), UpsertedAgents: make([]*Agent, len(w.UpsertedAgents)), DeletedWorkspaces: make([]*Workspace, len(w.DeletedWorkspaces)), DeletedAgents: make([]*Agent, len(w.DeletedAgents)), + Kind: w.Kind, } for i, ws := range w.UpsertedWorkspaces { clone.UpsertedWorkspaces[i] = &Workspace{ @@ -1115,7 +1142,7 @@ func (w *WorkspaceUpdate) Clone() WorkspaceUpdate { return clone } -func (t *tunnelUpdater) handleUpdate(update *proto.WorkspaceUpdate) error { +func (t *tunnelUpdater) handleUpdate(update *proto.WorkspaceUpdate, updateKind UpdateKind) error { t.Lock() defer t.Unlock() @@ -1124,6 +1151,7 @@ func (t *tunnelUpdater) handleUpdate(update *proto.WorkspaceUpdate) error { UpsertedAgents: []*Agent{}, DeletedWorkspaces: []*Workspace{}, DeletedAgents: []*Agent{}, + Kind: updateKind, } for _, uw := range update.UpsertedWorkspaces { diff --git a/tailnet/controllers_test.go b/tailnet/controllers_test.go index 67834de462655..bb5b543378ebe 100644 --- a/tailnet/controllers_test.go +++ b/tailnet/controllers_test.go @@ -1611,6 +1611,7 @@ func TestTunnelAllWorkspaceUpdatesController_Initial(t *testing.T) { }, DeletedWorkspaces: []*tailnet.Workspace{}, DeletedAgents: []*tailnet.Agent{}, + Kind: tailnet.Snapshot, } // And the callback @@ -1626,6 +1627,9 @@ func TestTunnelAllWorkspaceUpdatesController_Initial(t *testing.T) { slices.SortFunc(recvState.UpsertedAgents, func(a, b *tailnet.Agent) int { return strings.Compare(a.Name, b.Name) }) + // tunnel is still open, so it's a diff + currentState.Kind = tailnet.Diff + require.Equal(t, currentState, recvState) } @@ -1692,14 +1696,17 @@ func TestTunnelAllWorkspaceUpdatesController_DeleteAgent(t *testing.T) { }, DeletedWorkspaces: []*tailnet.Workspace{}, DeletedAgents: []*tailnet.Agent{}, + Kind: tailnet.Snapshot, } cbUpdate := testutil.TryReceive(ctx, t, fUH.ch) require.Equal(t, initRecvUp, cbUpdate) - // Current state should match initial state, err := updateCtrl.CurrentState() require.NoError(t, err) + // tunnel is still open, so it's a diff + initRecvUp.Kind = tailnet.Diff + require.Equal(t, initRecvUp, state) // Send update that removes w1a1 and adds w1a2 diff --git a/vpn/tunnel.go b/vpn/tunnel.go index 63de203980d14..6c71aecaa0965 100644 --- a/vpn/tunnel.go +++ b/vpn/tunnel.go @@ -88,6 +88,7 @@ func NewTunnel( netLoopDone: make(chan struct{}), uSendCh: s.sendCh, agents: map[uuid.UUID]tailnet.Agent{}, + workspaces: map[uuid.UUID]tailnet.Workspace{}, clock: quartz.NewReal(), }, } @@ -347,7 +348,9 @@ type updater struct { uSendCh chan<- *TunnelMessage // agents contains the agents that are currently connected to the tunnel. agents map[uuid.UUID]tailnet.Agent - conn Conn + // workspaces contains the workspaces to which agents are currently connected via the tunnel. + workspaces map[uuid.UUID]tailnet.Workspace + conn Conn clock quartz.Clock } @@ -397,6 +400,11 @@ func (u *updater) sendUpdateResponse(req *request[*TunnelMessage, *ManagerMessag // createPeerUpdateLocked creates a PeerUpdate message from a workspace update, populating // the network status of the agents. func (u *updater) createPeerUpdateLocked(update tailnet.WorkspaceUpdate) *PeerUpdate { + // if the update is a snapshot, we need to process the full state + if update.Kind == tailnet.Snapshot { + processSnapshotUpdate(&update, u.agents, u.workspaces) + } + out := &PeerUpdate{ UpsertedWorkspaces: make([]*Workspace, len(update.UpsertedWorkspaces)), UpsertedAgents: make([]*Agent, len(update.UpsertedAgents)), @@ -404,7 +412,20 @@ func (u *updater) createPeerUpdateLocked(update tailnet.WorkspaceUpdate) *PeerUp DeletedAgents: make([]*Agent, len(update.DeletedAgents)), } - u.saveUpdateLocked(update) + // save the workspace update to the tunnel's state, such that it can + // be used to populate automated peer updates. + for _, agent := range update.UpsertedAgents { + u.agents[agent.ID] = agent.Clone() + } + for _, agent := range update.DeletedAgents { + delete(u.agents, agent.ID) + } + for _, workspace := range update.UpsertedWorkspaces { + u.workspaces[workspace.ID] = workspace.Clone() + } + for _, workspace := range update.DeletedWorkspaces { + delete(u.workspaces, workspace.ID) + } for i, ws := range update.UpsertedWorkspaces { out.UpsertedWorkspaces[i] = &Workspace{ @@ -413,6 +434,7 @@ func (u *updater) createPeerUpdateLocked(update tailnet.WorkspaceUpdate) *PeerUp Status: Workspace_Status(ws.Status), } } + upsertedAgents := u.convertAgentsLocked(update.UpsertedAgents) out.UpsertedAgents = upsertedAgents for i, ws := range update.DeletedWorkspaces { @@ -472,17 +494,6 @@ func (u *updater) convertAgentsLocked(agents []*tailnet.Agent) []*Agent { return out } -// saveUpdateLocked saves the workspace update to the tunnel's state, such that it can -// be used to populate automated peer updates. -func (u *updater) saveUpdateLocked(update tailnet.WorkspaceUpdate) { - for _, agent := range update.UpsertedAgents { - u.agents[agent.ID] = agent.Clone() - } - for _, agent := range update.DeletedAgents { - delete(u.agents, agent.ID) - } -} - // setConn sets the `conn` and returns false if there's already a connection set. func (u *updater) setConn(conn Conn) bool { u.mu.Lock() @@ -552,6 +563,46 @@ func (u *updater) netStatusLoop() { } } +// processSnapshotUpdate handles the logic when a full state update is received. +// When the tunnel is live, we only receive diffs, but the first packet on any given +// reconnect to the tailnet API is a full state. +// Without this logic we weren't processing deletes for any workspaces or agents deleted +// while the client was disconnected while the computer was asleep. +func processSnapshotUpdate(update *tailnet.WorkspaceUpdate, agents map[uuid.UUID]tailnet.Agent, workspaces map[uuid.UUID]tailnet.Workspace) { + // ignoredWorkspaces is initially populated with the workspaces that are + // in the current update. Later on we populate it with the deleted workspaces too + // so that we don't send duplicate updates. Same applies to ignoredAgents. + ignoredWorkspaces := make(map[uuid.UUID]struct{}, len(update.UpsertedWorkspaces)) + ignoredAgents := make(map[uuid.UUID]struct{}, len(update.UpsertedAgents)) + + for _, workspace := range update.UpsertedWorkspaces { + ignoredWorkspaces[workspace.ID] = struct{}{} + } + for _, agent := range update.UpsertedAgents { + ignoredAgents[agent.ID] = struct{}{} + } + for _, agent := range agents { + if _, present := ignoredAgents[agent.ID]; !present { + // delete any current agents that are not in the new update + update.DeletedAgents = append(update.DeletedAgents, &tailnet.Agent{ + ID: agent.ID, + Name: agent.Name, + WorkspaceID: agent.WorkspaceID, + }) + } + } + for _, workspace := range workspaces { + if _, present := ignoredWorkspaces[workspace.ID]; !present { + update.DeletedWorkspaces = append(update.DeletedWorkspaces, &tailnet.Workspace{ + ID: workspace.ID, + Name: workspace.Name, + Status: workspace.Status, + }) + ignoredWorkspaces[workspace.ID] = struct{}{} + } + } +} + // hostsToIPStrings returns a slice of all unique IP addresses in the values // of the given map. func hostsToIPStrings(hosts map[dnsname.FQDN][]netip.Addr) []string { diff --git a/vpn/tunnel_internal_test.go b/vpn/tunnel_internal_test.go index d1d7377361f79..2beba66d7a7d2 100644 --- a/vpn/tunnel_internal_test.go +++ b/vpn/tunnel_internal_test.go @@ -2,6 +2,7 @@ package vpn import ( "context" + "maps" "net" "net/netip" "net/url" @@ -292,6 +293,7 @@ func TestUpdater_createPeerUpdate(t *testing.T) { ctx: ctx, netLoopDone: make(chan struct{}), agents: map[uuid.UUID]tailnet.Agent{}, + workspaces: map[uuid.UUID]tailnet.Workspace{}, conn: newFakeConn(tailnet.WorkspaceUpdate{}, hsTime), } @@ -486,6 +488,212 @@ func TestTunnel_sendAgentUpdate(t *testing.T) { require.Equal(t, hsTime, req.msg.GetPeerUpdate().UpsertedAgents[0].LastHandshake.AsTime()) } +func TestTunnel_sendAgentUpdateReconnect(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + + mClock := quartz.NewMock(t) + + wID1 := uuid.UUID{1} + aID1 := uuid.UUID{2} + aID2 := uuid.UUID{3} + + hsTime := time.Now().Add(-time.Minute).UTC() + + client := newFakeClient(ctx, t) + conn := newFakeConn(tailnet.WorkspaceUpdate{}, hsTime) + + tun, mgr := setupTunnel(t, ctx, client, mClock) + errCh := make(chan error, 1) + var resp *TunnelMessage + go func() { + r, err := mgr.unaryRPC(ctx, &ManagerMessage{ + Msg: &ManagerMessage_Start{ + Start: &StartRequest{ + TunnelFileDescriptor: 2, + CoderUrl: "https://coder.example.com", + ApiToken: "fakeToken", + }, + }, + }) + resp = r + errCh <- err + }() + testutil.RequireSend(ctx, t, client.ch, conn) + err := testutil.TryReceive(ctx, t, errCh) + require.NoError(t, err) + _, ok := resp.Msg.(*TunnelMessage_Start) + require.True(t, ok) + + // Inform the tunnel of the initial state + err = tun.Update(tailnet.WorkspaceUpdate{ + UpsertedWorkspaces: []*tailnet.Workspace{ + { + ID: wID1, Name: "w1", Status: proto.Workspace_STARTING, + }, + }, + UpsertedAgents: []*tailnet.Agent{ + { + ID: aID1, + Name: "agent1", + WorkspaceID: wID1, + Hosts: map[dnsname.FQDN][]netip.Addr{ + "agent1.coder.": {netip.MustParseAddr("fd60:627a:a42b:0101::")}, + }, + }, + }, + }) + require.NoError(t, err) + req := testutil.TryReceive(ctx, t, mgr.requests) + require.Nil(t, req.msg.Rpc) + require.NotNil(t, req.msg.GetPeerUpdate()) + require.Len(t, req.msg.GetPeerUpdate().UpsertedAgents, 1) + require.Equal(t, aID1[:], req.msg.GetPeerUpdate().UpsertedAgents[0].Id) + + // Upsert a new agent simulating a reconnect + err = tun.Update(tailnet.WorkspaceUpdate{ + UpsertedWorkspaces: []*tailnet.Workspace{ + { + ID: wID1, Name: "w1", Status: proto.Workspace_STARTING, + }, + }, + UpsertedAgents: []*tailnet.Agent{ + { + ID: aID2, + Name: "agent2", + WorkspaceID: wID1, + Hosts: map[dnsname.FQDN][]netip.Addr{ + "agent2.coder.": {netip.MustParseAddr("fd60:627a:a42b:0101::")}, + }, + }, + }, + Kind: tailnet.Snapshot, + }) + require.NoError(t, err) + + // The new update only contains the new agent + mClock.AdvanceNext() + req = testutil.TryReceive(ctx, t, mgr.requests) + require.Nil(t, req.msg.Rpc) + peerUpdate := req.msg.GetPeerUpdate() + require.NotNil(t, peerUpdate) + require.Len(t, peerUpdate.UpsertedAgents, 1) + require.Len(t, peerUpdate.DeletedAgents, 1) + require.Len(t, peerUpdate.DeletedWorkspaces, 0) + + require.Equal(t, aID2[:], peerUpdate.UpsertedAgents[0].Id) + require.Equal(t, hsTime, peerUpdate.UpsertedAgents[0].LastHandshake.AsTime()) + + require.Equal(t, aID1[:], peerUpdate.DeletedAgents[0].Id) +} + +func TestTunnel_sendAgentUpdateWorkspaceReconnect(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + + mClock := quartz.NewMock(t) + + wID1 := uuid.UUID{1} + wID2 := uuid.UUID{2} + aID1 := uuid.UUID{3} + aID3 := uuid.UUID{4} + + hsTime := time.Now().Add(-time.Minute).UTC() + + client := newFakeClient(ctx, t) + conn := newFakeConn(tailnet.WorkspaceUpdate{}, hsTime) + + tun, mgr := setupTunnel(t, ctx, client, mClock) + errCh := make(chan error, 1) + var resp *TunnelMessage + go func() { + r, err := mgr.unaryRPC(ctx, &ManagerMessage{ + Msg: &ManagerMessage_Start{ + Start: &StartRequest{ + TunnelFileDescriptor: 2, + CoderUrl: "https://coder.example.com", + ApiToken: "fakeToken", + }, + }, + }) + resp = r + errCh <- err + }() + testutil.RequireSend(ctx, t, client.ch, conn) + err := testutil.TryReceive(ctx, t, errCh) + require.NoError(t, err) + _, ok := resp.Msg.(*TunnelMessage_Start) + require.True(t, ok) + + // Inform the tunnel of the initial state + err = tun.Update(tailnet.WorkspaceUpdate{ + UpsertedWorkspaces: []*tailnet.Workspace{ + { + ID: wID1, Name: "w1", Status: proto.Workspace_STARTING, + }, + }, + UpsertedAgents: []*tailnet.Agent{ + { + ID: aID1, + Name: "agent1", + WorkspaceID: wID1, + Hosts: map[dnsname.FQDN][]netip.Addr{ + "agent1.coder.": {netip.MustParseAddr("fd60:627a:a42b:0101::")}, + }, + }, + }, + }) + require.NoError(t, err) + req := testutil.TryReceive(ctx, t, mgr.requests) + require.Nil(t, req.msg.Rpc) + require.NotNil(t, req.msg.GetPeerUpdate()) + require.Len(t, req.msg.GetPeerUpdate().UpsertedAgents, 1) + require.Equal(t, aID1[:], req.msg.GetPeerUpdate().UpsertedAgents[0].Id) + + // Upsert a new agent with a new workspace while simulating a reconnect + err = tun.Update(tailnet.WorkspaceUpdate{ + UpsertedWorkspaces: []*tailnet.Workspace{ + { + ID: wID2, Name: "w2", Status: proto.Workspace_STARTING, + }, + }, + UpsertedAgents: []*tailnet.Agent{ + { + ID: aID3, + Name: "agent3", + WorkspaceID: wID2, + Hosts: map[dnsname.FQDN][]netip.Addr{ + "agent3.coder.": {netip.MustParseAddr("fd60:627a:a42b:0101::")}, + }, + }, + }, + Kind: tailnet.Snapshot, + }) + require.NoError(t, err) + + // The new update only contains the new agent + mClock.AdvanceNext() + req = testutil.TryReceive(ctx, t, mgr.requests) + mClock.AdvanceNext() + + require.Nil(t, req.msg.Rpc) + peerUpdate := req.msg.GetPeerUpdate() + require.NotNil(t, peerUpdate) + require.Len(t, peerUpdate.UpsertedWorkspaces, 1) + require.Len(t, peerUpdate.UpsertedAgents, 1) + require.Len(t, peerUpdate.DeletedAgents, 1) + require.Len(t, peerUpdate.DeletedWorkspaces, 1) + + require.Equal(t, wID2[:], peerUpdate.UpsertedWorkspaces[0].Id) + require.Equal(t, aID3[:], peerUpdate.UpsertedAgents[0].Id) + require.Equal(t, hsTime, peerUpdate.UpsertedAgents[0].LastHandshake.AsTime()) + + require.Equal(t, aID1[:], peerUpdate.DeletedAgents[0].Id) + require.Equal(t, wID1[:], peerUpdate.DeletedWorkspaces[0].Id) +} + //nolint:revive // t takes precedence func setupTunnel(t *testing.T, ctx context.Context, client *fakeClient, mClock quartz.Clock) (*Tunnel, *speaker[*ManagerMessage, *TunnelMessage, TunnelMessage]) { mp, tp := net.Pipe() @@ -513,3 +721,191 @@ func setupTunnel(t *testing.T, ctx context.Context, client *fakeClient, mClock q mgr.start() return tun, mgr } + +func TestProcessFreshState(t *testing.T) { + t.Parallel() + + wsID1 := uuid.New() + wsID2 := uuid.New() + wsID3 := uuid.New() + wsID4 := uuid.New() + + agentID1 := uuid.New() + agentID2 := uuid.New() + agentID3 := uuid.New() + agentID4 := uuid.New() + + agent1 := tailnet.Agent{ID: agentID1, Name: "agent1", WorkspaceID: wsID1} + agent2 := tailnet.Agent{ID: agentID2, Name: "agent2", WorkspaceID: wsID2} + agent3 := tailnet.Agent{ID: agentID3, Name: "agent3", WorkspaceID: wsID3} + agent4 := tailnet.Agent{ID: agentID4, Name: "agent4", WorkspaceID: wsID1} + + ws1 := tailnet.Workspace{ID: wsID1, Name: "ws1", Status: proto.Workspace_RUNNING} + ws2 := tailnet.Workspace{ID: wsID2, Name: "ws2", Status: proto.Workspace_RUNNING} + ws3 := tailnet.Workspace{ID: wsID3, Name: "ws3", Status: proto.Workspace_RUNNING} + ws4 := tailnet.Workspace{ID: wsID4, Name: "ws4", Status: proto.Workspace_RUNNING} + + initialAgents := map[uuid.UUID]tailnet.Agent{ + agentID1: agent1, + agentID2: agent2, + agentID4: agent4, + } + initialWorkspaces := map[uuid.UUID]tailnet.Workspace{ + wsID1: ws1, + wsID2: ws2, + } + + tests := []struct { + name string + initialAgents map[uuid.UUID]tailnet.Agent + initialWorkspaces map[uuid.UUID]tailnet.Workspace + update *tailnet.WorkspaceUpdate + expectedDelete *tailnet.WorkspaceUpdate // We only care about deletions added by the function + }{ + { + name: "NoChange", + initialAgents: initialAgents, + initialWorkspaces: initialWorkspaces, + update: &tailnet.WorkspaceUpdate{ + Kind: tailnet.Snapshot, + UpsertedWorkspaces: []*tailnet.Workspace{&ws1, &ws2}, + UpsertedAgents: []*tailnet.Agent{&agent1, &agent2, &agent4}, + DeletedWorkspaces: []*tailnet.Workspace{}, + DeletedAgents: []*tailnet.Agent{}, + }, + expectedDelete: &tailnet.WorkspaceUpdate{ // Expect no *additional* deletions + DeletedWorkspaces: []*tailnet.Workspace{}, + DeletedAgents: []*tailnet.Agent{}, + }, + }, + { + name: "AgentAdded", // Agent 3 added in update + initialAgents: initialAgents, + initialWorkspaces: initialWorkspaces, + update: &tailnet.WorkspaceUpdate{ + Kind: tailnet.Snapshot, + UpsertedWorkspaces: []*tailnet.Workspace{&ws1, &ws2, &ws3}, + UpsertedAgents: []*tailnet.Agent{&agent1, &agent2, &agent3, &agent4}, + DeletedWorkspaces: []*tailnet.Workspace{}, + DeletedAgents: []*tailnet.Agent{}, + }, + expectedDelete: &tailnet.WorkspaceUpdate{ + DeletedWorkspaces: []*tailnet.Workspace{}, + DeletedAgents: []*tailnet.Agent{}, + }, + }, + { + name: "AgentRemovedWorkspaceAlsoRemoved", // Agent 2 removed, ws2 also removed + initialAgents: initialAgents, + initialWorkspaces: initialWorkspaces, + update: &tailnet.WorkspaceUpdate{ + Kind: tailnet.Snapshot, + UpsertedWorkspaces: []*tailnet.Workspace{&ws1}, // ws2 not present + UpsertedAgents: []*tailnet.Agent{&agent1, &agent4}, // agent2 not present + DeletedWorkspaces: []*tailnet.Workspace{}, + DeletedAgents: []*tailnet.Agent{}, + }, + expectedDelete: &tailnet.WorkspaceUpdate{ + DeletedWorkspaces: []*tailnet.Workspace{ + {ID: wsID2, Name: "ws2", Status: proto.Workspace_RUNNING}, + }, // Expect ws2 to be deleted + DeletedAgents: []*tailnet.Agent{ // Expect agent2 to be deleted + {ID: agentID2, Name: "agent2", WorkspaceID: wsID2}, + }, + }, + }, + { + name: "AgentRemovedWorkspaceStays", // Agent 4 removed, but ws1 stays (due to agent1) + initialAgents: initialAgents, + initialWorkspaces: initialWorkspaces, + update: &tailnet.WorkspaceUpdate{ + Kind: tailnet.Snapshot, + UpsertedWorkspaces: []*tailnet.Workspace{&ws1, &ws2}, // ws1 still present + UpsertedAgents: []*tailnet.Agent{&agent1, &agent2}, // agent4 not present + DeletedWorkspaces: []*tailnet.Workspace{}, + DeletedAgents: []*tailnet.Agent{}, + }, + expectedDelete: &tailnet.WorkspaceUpdate{ + DeletedWorkspaces: []*tailnet.Workspace{}, // ws1 should NOT be deleted + DeletedAgents: []*tailnet.Agent{ // Expect agent4 to be deleted + {ID: agentID4, Name: "agent4", WorkspaceID: wsID1}, + }, + }, + }, + { + name: "InitialAgentsEmpty", + initialAgents: map[uuid.UUID]tailnet.Agent{}, // Start with no agents known + initialWorkspaces: map[uuid.UUID]tailnet.Workspace{}, + update: &tailnet.WorkspaceUpdate{ + Kind: tailnet.Snapshot, + UpsertedWorkspaces: []*tailnet.Workspace{&ws1, &ws2}, + UpsertedAgents: []*tailnet.Agent{&agent1, &agent2}, + DeletedWorkspaces: []*tailnet.Workspace{}, + DeletedAgents: []*tailnet.Agent{}, + }, + expectedDelete: &tailnet.WorkspaceUpdate{ // Expect no deletions added + DeletedWorkspaces: []*tailnet.Workspace{}, + DeletedAgents: []*tailnet.Agent{}, + }, + }, + { + name: "UpdateEmpty", // Snapshot says nothing exists + initialAgents: initialAgents, + initialWorkspaces: initialWorkspaces, + update: &tailnet.WorkspaceUpdate{ + Kind: tailnet.Snapshot, + UpsertedWorkspaces: []*tailnet.Workspace{}, + UpsertedAgents: []*tailnet.Agent{}, + DeletedWorkspaces: []*tailnet.Workspace{}, + DeletedAgents: []*tailnet.Agent{}, + }, + expectedDelete: &tailnet.WorkspaceUpdate{ // Expect all initial agents/workspaces to be deleted + DeletedWorkspaces: []*tailnet.Workspace{ + {ID: wsID1, Name: "ws1", Status: proto.Workspace_RUNNING}, + {ID: wsID2, Name: "ws2", Status: proto.Workspace_RUNNING}, + }, // ws1 and ws2 deleted + DeletedAgents: []*tailnet.Agent{ // agent1, agent2, agent4 deleted + {ID: agentID1, Name: "agent1", WorkspaceID: wsID1}, + {ID: agentID2, Name: "agent2", WorkspaceID: wsID2}, + {ID: agentID4, Name: "agent4", WorkspaceID: wsID1}, + }, + }, + }, + { + name: "WorkspaceWithNoAgents", // Snapshot says nothing exists + initialAgents: initialAgents, + initialWorkspaces: map[uuid.UUID]tailnet.Workspace{wsID1: ws1, wsID2: ws2, wsID4: ws4}, // ws4 has no agents + update: &tailnet.WorkspaceUpdate{ + Kind: tailnet.Snapshot, + UpsertedWorkspaces: []*tailnet.Workspace{&ws1, &ws2}, + UpsertedAgents: []*tailnet.Agent{&agent1, &agent2, &agent4}, + DeletedWorkspaces: []*tailnet.Workspace{}, + DeletedAgents: []*tailnet.Agent{}, + }, + expectedDelete: &tailnet.WorkspaceUpdate{ // Expect all initial agents/workspaces to be deleted + DeletedWorkspaces: []*tailnet.Workspace{ + {ID: wsID4, Name: "ws4", Status: proto.Workspace_RUNNING}, + }, // ws4 should be deleted + DeletedAgents: []*tailnet.Agent{}, + }, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + agentsCopy := make(map[uuid.UUID]tailnet.Agent) + maps.Copy(agentsCopy, tt.initialAgents) + + workspaceCopy := make(map[uuid.UUID]tailnet.Workspace) + maps.Copy(workspaceCopy, tt.initialWorkspaces) + + processSnapshotUpdate(tt.update, agentsCopy, workspaceCopy) + + require.ElementsMatch(t, tt.expectedDelete.DeletedAgents, tt.update.DeletedAgents, "DeletedAgents mismatch") + require.ElementsMatch(t, tt.expectedDelete.DeletedWorkspaces, tt.update.DeletedWorkspaces, "DeletedWorkspaces mismatch") + }) + } +} From d9b00e48495bdaee660b9548fdca7fb6829e99c9 Mon Sep 17 00:00:00 2001 From: Bruno Quaresma Date: Tue, 6 May 2025 11:28:14 -0300 Subject: [PATCH 2/5] feat: add inline actions into workspaces table (#17636) Related to https://github.com/coder/coder/issues/17311 This PR adds inline actions in the workspaces page. It is a bit different of the [original design](https://www.figma.com/design/OR75XeUI0Z3ksqt1mHsNQw/Workspace-views?node-id=656-3979&m=dev) because I'm splitting the work into three phases that I will explain in more details in the demo. https://github.com/user-attachments/assets/6383375e-ed10-45d1-b5d5-b4421e86d158 --- site/src/api/queries/workspaces.ts | 11 +- site/src/components/Dialogs/Dialog.tsx | 9 +- site/src/hooks/usePagination.ts | 6 +- .../workspaces/WorkspaceUpdateDialogs.tsx | 155 ++++++++++++ .../workspaces/actions.ts} | 16 +- .../useWorkspacesToBeDeleted.ts | 9 +- .../pages/WorkspacePage/Workspace.stories.tsx | 4 +- site/src/pages/WorkspacePage/Workspace.tsx | 3 - .../WorkspaceActions.stories.tsx | 15 +- .../WorkspaceActions/WorkspaceActions.tsx | 18 +- .../WorkspacePage/WorkspaceReadyPage.tsx | 71 +----- .../WorkspacePage/WorkspaceTopbar.stories.tsx | 5 +- .../pages/WorkspacePage/WorkspaceTopbar.tsx | 3 - .../WorkspacesPage/WorkspacesPage.test.tsx | 4 +- .../pages/WorkspacesPage/WorkspacesPage.tsx | 18 +- .../WorkspacesPageView.stories.tsx | 4 +- .../WorkspacesPage/WorkspacesPageView.tsx | 6 + .../pages/WorkspacesPage/WorkspacesTable.tsx | 239 +++++++++++++++++- site/src/pages/WorkspacesPage/data.ts | 26 +- 19 files changed, 495 insertions(+), 127 deletions(-) create mode 100644 site/src/modules/workspaces/WorkspaceUpdateDialogs.tsx rename site/src/{pages/WorkspacePage/WorkspaceActions/constants.ts => modules/workspaces/actions.ts} (90%) diff --git a/site/src/api/queries/workspaces.ts b/site/src/api/queries/workspaces.ts index fd840d067821a..39008f5c712a3 100644 --- a/site/src/api/queries/workspaces.ts +++ b/site/src/api/queries/workspaces.ts @@ -139,13 +139,9 @@ function workspacesKey(config: WorkspacesRequest = {}) { } export function workspaces(config: WorkspacesRequest = {}) { - // Duplicates some of the work from workspacesKey, but that felt better than - // letting invisible properties sneak into the query logic - const { q, limit } = config; - return { queryKey: workspacesKey(config), - queryFn: () => API.getWorkspaces({ q, limit }), + queryFn: () => API.getWorkspaces(config), } as const satisfies QueryOptions; } @@ -281,7 +277,10 @@ const updateWorkspaceBuild = async ( build.workspace_owner_name, build.workspace_name, ); - const previousData = queryClient.getQueryData(workspaceKey) as Workspace; + const previousData = queryClient.getQueryData(workspaceKey); + if (!previousData) { + return; + } // Check if the build returned is newer than the previous build that could be // updated from web socket diff --git a/site/src/components/Dialogs/Dialog.tsx b/site/src/components/Dialogs/Dialog.tsx index cdc271697c680..532b47a1339dc 100644 --- a/site/src/components/Dialogs/Dialog.tsx +++ b/site/src/components/Dialogs/Dialog.tsx @@ -35,7 +35,14 @@ export const DialogActionButtons: FC = ({ return ( <> {onCancel && ( - )} diff --git a/site/src/hooks/usePagination.ts b/site/src/hooks/usePagination.ts index 72ea70868fb30..2ab409e85c9d8 100644 --- a/site/src/hooks/usePagination.ts +++ b/site/src/hooks/usePagination.ts @@ -9,7 +9,7 @@ export const usePagination = ({ const [searchParams, setSearchParams] = searchParamsResult; const page = searchParams.get("page") ? Number(searchParams.get("page")) : 1; const limit = DEFAULT_RECORDS_PER_PAGE; - const offset = page <= 0 ? 0 : (page - 1) * limit; + const offset = calcOffset(page, limit); const goToPage = (page: number) => { searchParams.set("page", page.toString()); @@ -23,3 +23,7 @@ export const usePagination = ({ offset, }; }; + +export const calcOffset = (page: number, limit: number) => { + return page <= 0 ? 0 : (page - 1) * limit; +}; diff --git a/site/src/modules/workspaces/WorkspaceUpdateDialogs.tsx b/site/src/modules/workspaces/WorkspaceUpdateDialogs.tsx new file mode 100644 index 0000000000000..741bc12a6539b --- /dev/null +++ b/site/src/modules/workspaces/WorkspaceUpdateDialogs.tsx @@ -0,0 +1,155 @@ +import { MissingBuildParameters } from "api/api"; +import { updateWorkspace } from "api/queries/workspaces"; +import type { + TemplateVersion, + Workspace, + WorkspaceBuild, + WorkspaceBuildParameter, +} from "api/typesGenerated"; +import { ConfirmDialog } from "components/Dialogs/ConfirmDialog/ConfirmDialog"; +import { MemoizedInlineMarkdown } from "components/Markdown/Markdown"; +import { UpdateBuildParametersDialog } from "pages/WorkspacePage/UpdateBuildParametersDialog"; +import { type FC, useState } from "react"; +import { useMutation, useQueryClient } from "react-query"; + +type UseWorkspaceUpdateOptions = { + workspace: Workspace; + latestVersion: TemplateVersion | undefined; + onSuccess?: (build: WorkspaceBuild) => void; + onError?: (error: unknown) => void; +}; + +type UseWorkspaceUpdateResult = { + update: () => void; + isUpdating: boolean; + dialogs: { + updateConfirmation: UpdateConfirmationDialogProps; + missingBuildParameters: MissingBuildParametersDialogProps; + }; +}; + +export const useWorkspaceUpdate = ({ + workspace, + latestVersion, + onSuccess, + onError, +}: UseWorkspaceUpdateOptions): UseWorkspaceUpdateResult => { + const queryClient = useQueryClient(); + const [isConfirmingUpdate, setIsConfirmingUpdate] = useState(false); + + const updateWorkspaceOptions = updateWorkspace(workspace, queryClient); + const updateWorkspaceMutation = useMutation({ + ...updateWorkspaceOptions, + onSuccess: (build: WorkspaceBuild) => { + updateWorkspaceOptions.onSuccess(build); + onSuccess?.(build); + }, + onError, + }); + + const update = () => { + setIsConfirmingUpdate(true); + }; + + const confirmUpdate = (buildParameters: WorkspaceBuildParameter[] = []) => { + updateWorkspaceMutation.mutate(buildParameters); + setIsConfirmingUpdate(false); + }; + + return { + update, + isUpdating: updateWorkspaceMutation.isLoading, + dialogs: { + updateConfirmation: { + open: isConfirmingUpdate, + onClose: () => setIsConfirmingUpdate(false), + onConfirm: () => confirmUpdate(), + latestVersion, + }, + missingBuildParameters: { + error: updateWorkspaceMutation.error, + onClose: () => { + updateWorkspaceMutation.reset(); + }, + onUpdate: (buildParameters: WorkspaceBuildParameter[]) => { + if (updateWorkspaceMutation.error instanceof MissingBuildParameters) { + confirmUpdate(buildParameters); + } + }, + }, + }, + }; +}; + +type WorkspaceUpdateDialogsProps = { + updateConfirmation: UpdateConfirmationDialogProps; + missingBuildParameters: MissingBuildParametersDialogProps; +}; + +export const WorkspaceUpdateDialogs: FC = ({ + updateConfirmation, + missingBuildParameters, +}) => { + return ( + <> + + + + ); +}; + +type UpdateConfirmationDialogProps = { + open: boolean; + onClose: () => void; + onConfirm: () => void; + latestVersion?: TemplateVersion; +}; + +const UpdateConfirmationDialog: FC = ({ + latestVersion, + ...dialogProps +}) => { + return ( + +

+ Updating your workspace will start the workspace on the latest + template version. This can{" "} + delete non-persistent data. +

+ {latestVersion?.message && ( + + {latestVersion.message} + + )} + + } + /> + ); +}; + +type MissingBuildParametersDialogProps = { + error: unknown; + onClose: () => void; + onUpdate: (buildParameters: WorkspaceBuildParameter[]) => void; +}; + +const MissingBuildParametersDialog: FC = ({ + error, + ...dialogProps +}) => { + return ( + + ); +}; diff --git a/site/src/pages/WorkspacePage/WorkspaceActions/constants.ts b/site/src/modules/workspaces/actions.ts similarity index 90% rename from site/src/pages/WorkspacePage/WorkspaceActions/constants.ts rename to site/src/modules/workspaces/actions.ts index a327f0277d4f5..6a255e2cd2c88 100644 --- a/site/src/pages/WorkspacePage/WorkspaceActions/constants.ts +++ b/site/src/modules/workspaces/actions.ts @@ -34,6 +34,11 @@ const actionTypes = [ export type ActionType = (typeof actionTypes)[number]; +type ActionPermissions = { + canDebug: boolean; + isOwner: boolean; +}; + type WorkspaceAbilities = { actions: readonly ActionType[]; canCancel: boolean; @@ -42,8 +47,11 @@ type WorkspaceAbilities = { export const abilitiesByWorkspaceStatus = ( workspace: Workspace, - canDebug: boolean, + permissions: ActionPermissions, ): WorkspaceAbilities => { + const hasPermissionToCancel = + workspace.template_allow_user_cancel_workspace_jobs || permissions.isOwner; + if (workspace.dormant_at) { return { actions: ["activate"], @@ -58,7 +66,7 @@ export const abilitiesByWorkspaceStatus = ( case "starting": { return { actions: ["starting"], - canCancel: true, + canCancel: hasPermissionToCancel, canAcceptJobs: false, }; } @@ -83,7 +91,7 @@ export const abilitiesByWorkspaceStatus = ( case "stopping": { return { actions: ["stopping"], - canCancel: true, + canCancel: hasPermissionToCancel, canAcceptJobs: false, }; } @@ -115,7 +123,7 @@ export const abilitiesByWorkspaceStatus = ( case "failed": { const actions: ActionType[] = ["retry"]; - if (canDebug) { + if (permissions.canDebug) { actions.push("debug"); } diff --git a/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/useWorkspacesToBeDeleted.ts b/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/useWorkspacesToBeDeleted.ts index 338c157f4f791..5a974d5d8fe31 100644 --- a/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/useWorkspacesToBeDeleted.ts +++ b/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/useWorkspacesToBeDeleted.ts @@ -1,5 +1,6 @@ import type { Template, Workspace } from "api/typesGenerated"; import { compareAsc } from "date-fns"; +import { calcOffset } from "hooks/usePagination"; import { useWorkspacesData } from "pages/WorkspacesPage/data"; import type { TemplateScheduleFormValues } from "./formHelpers"; @@ -9,9 +10,9 @@ export const useWorkspacesToGoDormant = ( fromDate: Date, ) => { const { data } = useWorkspacesData({ - page: 0, + offset: calcOffset(0, 0), limit: 0, - query: `template:${template.name}`, + q: `template:${template.name}`, }); return data?.workspaces?.filter((workspace: Workspace) => { @@ -40,9 +41,9 @@ export const useWorkspacesToBeDeleted = ( fromDate: Date, ) => { const { data } = useWorkspacesData({ - page: 0, + offset: calcOffset(0, 0), limit: 0, - query: `template:${template.name} dormant:true`, + q: `template:${template.name} dormant:true`, }); return data?.workspaces?.filter((workspace: Workspace) => { if (!workspace.dormant_at || !formValues.time_til_dormant_autodelete_ms) { diff --git a/site/src/pages/WorkspacePage/Workspace.stories.tsx b/site/src/pages/WorkspacePage/Workspace.stories.tsx index 88198bdb7b09a..7d29b02c11cb6 100644 --- a/site/src/pages/WorkspacePage/Workspace.stories.tsx +++ b/site/src/pages/WorkspacePage/Workspace.stories.tsx @@ -3,7 +3,7 @@ import type { Meta, StoryObj } from "@storybook/react"; import type { ProvisionerJobLog } from "api/typesGenerated"; import { ProxyContext, getPreferredProxy } from "contexts/ProxyContext"; import * as Mocks from "testHelpers/entities"; -import { withDashboardProvider } from "testHelpers/storybook"; +import { withAuthProvider, withDashboardProvider } from "testHelpers/storybook"; import { Workspace } from "./Workspace"; import type { WorkspacePermissions } from "./permissions"; @@ -40,8 +40,10 @@ const meta: Meta = { data: Mocks.MockListeningPortsResponse, }, ], + user: Mocks.MockUser, }, decorators: [ + withAuthProvider, withDashboardProvider, (Story) => ( = ({ buildLogs, latestVersion, permissions, - isOwner, timings, }) => { const navigate = useNavigate(); @@ -161,7 +159,6 @@ export const Workspace: FC = ({ isUpdating={isUpdating} isRestarting={isRestarting} canUpdateWorkspace={permissions.updateWorkspace} - isOwner={isOwner} template={template} permissions={permissions} latestVersion={latestVersion} diff --git a/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.stories.tsx b/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.stories.tsx index d726a047b7c57..48dda92b49503 100644 --- a/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.stories.tsx +++ b/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.stories.tsx @@ -3,6 +3,7 @@ import { expect, userEvent, within } from "@storybook/test"; import { agentLogsKey, buildLogsKey } from "api/queries/workspaces"; import * as Mocks from "testHelpers/entities"; import { + withAuthProvider, withDashboardProvider, withDesktopViewport, } from "testHelpers/storybook"; @@ -14,7 +15,10 @@ const meta: Meta = { args: { isUpdating: false, }, - decorators: [withDashboardProvider, withDesktopViewport], + decorators: [withDashboardProvider, withDesktopViewport, withAuthProvider], + parameters: { + user: Mocks.MockUser, + }, }; export default meta; @@ -163,14 +167,15 @@ export const CancelShownForOwner: Story = { ...Mocks.MockStartingWorkspace, template_allow_user_cancel_workspace_jobs: false, }, - isOwner: true, }, }; export const CancelShownForUser: Story = { args: { workspace: Mocks.MockStartingWorkspace, - isOwner: false, + }, + parameters: { + user: Mocks.MockUser2, }, }; @@ -180,7 +185,9 @@ export const CancelHiddenForUser: Story = { ...Mocks.MockStartingWorkspace, template_allow_user_cancel_workspace_jobs: false, }, - isOwner: false, + }, + parameters: { + user: Mocks.MockUser2, }, }; diff --git a/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.tsx b/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.tsx index 71f890cff3a5b..b65407806ed69 100644 --- a/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.tsx +++ b/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.tsx @@ -12,7 +12,12 @@ import { DropdownMenuSeparator, DropdownMenuTrigger, } from "components/DropdownMenu/DropdownMenu"; +import { useAuthenticated } from "hooks/useAuthenticated"; import { EllipsisVertical } from "lucide-react"; +import { + type ActionType, + abilitiesByWorkspaceStatus, +} from "modules/workspaces/actions"; import { useWorkspaceDuplication } from "pages/CreateWorkspacePage/useWorkspaceDuplication"; import { type FC, Fragment, type ReactNode, useState } from "react"; import { mustUpdateWorkspace } from "utils/workspace"; @@ -31,7 +36,6 @@ import { import { DebugButton } from "./DebugButton"; import { DownloadLogsDialog } from "./DownloadLogsDialog"; import { RetryButton } from "./RetryButton"; -import { type ActionType, abilitiesByWorkspaceStatus } from "./constants"; export interface WorkspaceActionsProps { workspace: Workspace; @@ -52,7 +56,6 @@ export interface WorkspaceActionsProps { children?: ReactNode; canChangeVersions: boolean; canDebug: boolean; - isOwner: boolean; } export const WorkspaceActions: FC = ({ @@ -73,20 +76,19 @@ export const WorkspaceActions: FC = ({ isRestarting, canChangeVersions, canDebug, - isOwner, }) => { const { duplicateWorkspace, isDuplicationReady } = useWorkspaceDuplication(workspace); const [isDownloadDialogOpen, setIsDownloadDialogOpen] = useState(false); + const { user } = useAuthenticated(); + const isOwner = + user.roles.find((role) => role.name === "owner") !== undefined; const { actions, canCancel, canAcceptJobs } = abilitiesByWorkspaceStatus( workspace, - canDebug, + { canDebug, isOwner }, ); - const showCancel = - canCancel && - (workspace.template_allow_user_cancel_workspace_jobs || isOwner); const mustUpdate = mustUpdateWorkspace(workspace, canChangeVersions); const tooltipText = getTooltipText(workspace, mustUpdate, canChangeVersions); @@ -169,7 +171,7 @@ export const WorkspaceActions: FC = ({ {buttonMapping[action]} ))} - {showCancel && } + {canCancel && } = ({ throw Error("Workspace is undefined"); } - // Owner - const { user: me } = useAuthenticated(); - const isOwner = me.roles.find((role) => role.name === "owner") !== undefined; - // Debug mode const { data: deploymentValues } = useQuery({ ...deploymentConfig(), @@ -120,10 +116,10 @@ export const WorkspaceReadyPage: FC = ({ }); // Update workspace - const [isConfirmingUpdate, setIsConfirmingUpdate] = useState(false); - const updateWorkspaceMutation = useMutation( - updateWorkspace(workspace, queryClient), - ); + const workspaceUpdate = useWorkspaceUpdate({ + workspace, + latestVersion, + }); // If a user can update the template then they can force a delete // (via orphan). @@ -233,7 +229,7 @@ export const WorkspaceReadyPage: FC = ({ { @@ -248,9 +244,7 @@ export const WorkspaceReadyPage: FC = ({ handleRestart={(buildParameters) => { setConfirmingRestart({ open: true, buildParameters }); }} - handleUpdate={() => { - setIsConfirmingUpdate(true); - }} + handleUpdate={workspaceUpdate.update} handleCancel={cancelBuildMutation.mutate} handleSettings={() => navigate("settings")} handleRetry={handleRetry} @@ -280,7 +274,6 @@ export const WorkspaceReadyPage: FC = ({ sshPrefix={sshPrefixQuery.data?.hostname_prefix} template={template} buildLogs={buildLogs} - isOwner={isOwner} timings={timingsQuery.data} /> @@ -318,23 +311,6 @@ export const WorkspaceReadyPage: FC = ({ }} /> - { - updateWorkspaceMutation.reset(); - }} - onUpdate={(buildParameters) => { - if (updateWorkspaceMutation.error instanceof MissingBuildParameters) { - updateWorkspaceMutation.mutate(buildParameters); - } - }} - /> - = ({ }} /> - { - updateWorkspaceMutation.mutate(undefined); - setIsConfirmingUpdate(false); - }} - onClose={() => setIsConfirmingUpdate(false)} - title="Update workspace?" - confirmText="Update" - description={ - -

- Updating your workspace will start the workspace on the latest - template version. This can{" "} - delete non-persistent data. -

- {latestVersion?.message && ( - - {latestVersion.message} - - )} - - } - /> - { @@ -395,6 +346,8 @@ export const WorkspaceReadyPage: FC = ({ } /> + + ); }; diff --git a/site/src/pages/WorkspacePage/WorkspaceTopbar.stories.tsx b/site/src/pages/WorkspacePage/WorkspaceTopbar.stories.tsx index ce2ad840a1df0..84af8a518acd8 100644 --- a/site/src/pages/WorkspacePage/WorkspaceTopbar.stories.tsx +++ b/site/src/pages/WorkspacePage/WorkspaceTopbar.stories.tsx @@ -10,7 +10,7 @@ import { MockUser, MockWorkspace, } from "testHelpers/entities"; -import { withDashboardProvider } from "testHelpers/storybook"; +import { withAuthProvider, withDashboardProvider } from "testHelpers/storybook"; import { WorkspaceTopbar } from "./WorkspaceTopbar"; // We want a workspace without a deadline to not pollute the screenshot. Also @@ -28,7 +28,7 @@ const baseWorkspace: Workspace = { const meta: Meta = { title: "pages/WorkspacePage/WorkspaceTopbar", component: WorkspaceTopbar, - decorators: [withDashboardProvider], + decorators: [withAuthProvider, withDashboardProvider], args: { workspace: baseWorkspace, template: MockTemplate, @@ -41,6 +41,7 @@ const meta: Meta = { chromatic: { diffThreshold: 0.6, }, + user: MockUser, }, }; diff --git a/site/src/pages/WorkspacePage/WorkspaceTopbar.tsx b/site/src/pages/WorkspacePage/WorkspaceTopbar.tsx index 2492e50e7c5d6..a39cf6d8b13ca 100644 --- a/site/src/pages/WorkspacePage/WorkspaceTopbar.tsx +++ b/site/src/pages/WorkspacePage/WorkspaceTopbar.tsx @@ -55,7 +55,6 @@ export interface WorkspaceProps { canDebugMode: boolean; handleRetry: (buildParameters?: TypesGen.WorkspaceBuildParameter[]) => void; handleDebug: (buildParameters?: TypesGen.WorkspaceBuildParameter[]) => void; - isOwner: boolean; template: TypesGen.Template; permissions: WorkspacePermissions; latestVersion?: TypesGen.TemplateVersion; @@ -81,7 +80,6 @@ export const WorkspaceTopbar: FC = ({ canDebugMode, handleRetry, handleDebug, - isOwner, template, latestVersion, permissions, @@ -262,7 +260,6 @@ export const WorkspaceTopbar: FC = ({ canChangeVersions={canChangeVersions} isUpdating={isUpdating} isRestarting={isRestarting} - isOwner={isOwner} /> )} diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.test.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.test.tsx index 66388eb3f7dd1..b1ad1d887e53c 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPage.test.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPage.test.tsx @@ -264,7 +264,7 @@ describe("WorkspacesPage", () => { await user.click(getWorkspaceCheckbox(workspaces[0])); await user.click(getWorkspaceCheckbox(workspaces[1])); await user.click(screen.getByRole("button", { name: /actions/i })); - const stopButton = await screen.findByText(/stop/i); + const stopButton = await screen.findByRole("menuitem", { name: /stop/i }); await user.click(stopButton); await waitFor(() => { @@ -291,7 +291,7 @@ describe("WorkspacesPage", () => { await user.click(getWorkspaceCheckbox(workspaces[0])); await user.click(getWorkspaceCheckbox(workspaces[1])); await user.click(screen.getByRole("button", { name: /actions/i })); - const startButton = await screen.findByText(/start/i); + const startButton = await screen.findByRole("menuitem", { name: /start/i }); await user.click(startButton); await waitFor(() => { diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx index ba380905adda2..551c554fd5ee3 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx @@ -1,8 +1,10 @@ +import { getErrorDetail, getErrorMessage } from "api/errors"; import { workspacePermissionsByOrganization } from "api/queries/organizations"; import { templates } from "api/queries/templates"; import type { Workspace } from "api/typesGenerated"; import { useFilter } from "components/Filter/Filter"; import { useUserFilterMenu } from "components/Filter/UserFilter"; +import { displayError } from "components/GlobalSnackbar/utils"; import { useAuthenticated } from "hooks"; import { useEffectEvent } from "hooks/hookPolyfills"; import { usePagination } from "hooks/usePagination"; @@ -10,7 +12,7 @@ import { useDashboard } from "modules/dashboard/useDashboard"; import { useOrganizationsFilterMenu } from "modules/tableFiltering/options"; import { type FC, useEffect, useMemo, useState } from "react"; import { Helmet } from "react-helmet-async"; -import { useQuery } from "react-query"; +import { useQuery, useQueryClient } from "react-query"; import { useSearchParams } from "react-router-dom"; import { pageTitle } from "utils/page"; import { BatchDeleteConfirmation } from "./BatchDeleteConfirmation"; @@ -35,6 +37,7 @@ function useSafeSearchParams() { } const WorkspacesPage: FC = () => { + const queryClient = useQueryClient(); // If we use a useSearchParams for each hook, the values will not be in sync. // So we have to use a single one, centralizing the values, and pass it to // each hook. @@ -72,7 +75,7 @@ const WorkspacesPage: FC = () => { const { data, error, queryKey, refetch } = useWorkspacesData({ ...pagination, - query: filterProps.filter.query, + q: filterProps.filter.query, }); const updateWorkspace = useWorkspaceUpdate(queryKey); @@ -128,6 +131,17 @@ const WorkspacesPage: FC = () => { onUpdateAll={() => setConfirmingBatchAction("update")} onStartAll={() => batchActions.startAll(checkedWorkspaces)} onStopAll={() => batchActions.stopAll(checkedWorkspaces)} + onActionSuccess={async () => { + await queryClient.invalidateQueries({ + queryKey, + }); + }} + onActionError={(error) => { + displayError( + getErrorMessage(error, "Failed to perform action"), + getErrorDetail(error), + ); + }} /> = { data: MockBuildInfo, }, ], + user: MockUser, }, decorators: [ + withAuthProvider, withDashboardProvider, (Story) => ( Promise; + onActionError: (error: unknown) => void; } export const WorkspacesPageView: FC = ({ @@ -88,6 +90,8 @@ export const WorkspacesPageView: FC = ({ templatesFetchStatus, canCreateTemplate, canChangeVersions, + onActionSuccess, + onActionError, }) => { // Let's say the user has 5 workspaces, but tried to hit page 100, which does // not exist. In this case, the page is not valid and we want to show a better @@ -224,6 +228,8 @@ export const WorkspacesPageView: FC = ({ onCheckChange={onCheckChange} canCheckWorkspaces={canCheckWorkspaces} templates={templates} + onActionSuccess={onActionSuccess} + onActionError={onActionError} /> )} diff --git a/site/src/pages/WorkspacesPage/WorkspacesTable.tsx b/site/src/pages/WorkspacesPage/WorkspacesTable.tsx index a9d585fccf58c..2fe94e0260a8f 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesTable.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesTable.tsx @@ -2,6 +2,13 @@ import KeyboardArrowRight from "@mui/icons-material/KeyboardArrowRight"; import Star from "@mui/icons-material/Star"; import Checkbox from "@mui/material/Checkbox"; import Skeleton from "@mui/material/Skeleton"; +import { templateVersion } from "api/queries/templates"; +import { + cancelBuild, + deleteWorkspace, + startWorkspace, + stopWorkspace, +} from "api/queries/workspaces"; import type { Template, Workspace, @@ -11,7 +18,9 @@ import type { import { Avatar } from "components/Avatar/Avatar"; import { AvatarData } from "components/Avatar/AvatarData"; import { AvatarDataSkeleton } from "components/Avatar/AvatarDataSkeleton"; +import { Button } from "components/Button/Button"; import { InfoTooltip } from "components/InfoTooltip/InfoTooltip"; +import { Spinner } from "components/Spinner/Spinner"; import { Stack } from "components/Stack/Stack"; import { StatusIndicator, @@ -30,14 +39,33 @@ import { TableLoaderSkeleton, TableRowSkeleton, } from "components/TableLoader/TableLoader"; +import { + Tooltip, + TooltipContent, + TooltipProvider, + TooltipTrigger, +} from "components/Tooltip/Tooltip"; import dayjs from "dayjs"; import relativeTime from "dayjs/plugin/relativeTime"; +import { useAuthenticated } from "hooks"; import { useClickableTableRow } from "hooks/useClickableTableRow"; +import { BanIcon, PlayIcon, RefreshCcwIcon, SquareIcon } from "lucide-react"; import { useDashboard } from "modules/dashboard/useDashboard"; import { WorkspaceAppStatus } from "modules/workspaces/WorkspaceAppStatus/WorkspaceAppStatus"; import { WorkspaceDormantBadge } from "modules/workspaces/WorkspaceDormantBadge/WorkspaceDormantBadge"; import { WorkspaceOutdatedTooltip } from "modules/workspaces/WorkspaceOutdatedTooltip/WorkspaceOutdatedTooltip"; -import { type FC, type ReactNode, useMemo } from "react"; +import { + WorkspaceUpdateDialogs, + useWorkspaceUpdate, +} from "modules/workspaces/WorkspaceUpdateDialogs"; +import { abilitiesByWorkspaceStatus } from "modules/workspaces/actions"; +import { + type FC, + type PropsWithChildren, + type ReactNode, + useMemo, +} from "react"; +import { useMutation, useQuery, useQueryClient } from "react-query"; import { useNavigate } from "react-router-dom"; import { cn } from "utils/cn"; import { @@ -60,6 +88,8 @@ export interface WorkspacesTableProps { canCheckWorkspaces: boolean; templates?: Template[]; canCreateTemplate: boolean; + onActionSuccess: () => Promise; + onActionError: (error: unknown) => void; } export const WorkspacesTable: FC = ({ @@ -71,6 +101,8 @@ export const WorkspacesTable: FC = ({ canCheckWorkspaces, templates, canCreateTemplate, + onActionSuccess, + onActionError, }) => { const dashboard = useDashboard(); const workspaceIDToAppByStatus = useMemo(() => { @@ -139,6 +171,7 @@ export const WorkspacesTable: FC = ({ Template Status + @@ -260,10 +293,14 @@ export const WorkspacesTable: FC = ({ - + -
- +
+
@@ -289,7 +326,7 @@ const WorkspacesRow: FC = ({ const workspacePageLink = `/@${workspace.owner_name}/${workspace.name}`; const openLinkInNewTab = () => window.open(workspacePageLink, "_blank"); - const clickableProps = useClickableTableRow({ + const { role, hover, ...clickableProps } = useClickableTableRow({ onMiddleClick: openLinkInNewTab, onClick: (event) => { // Order of booleans actually matters here for Windows-Mac compatibility; @@ -341,7 +378,12 @@ const TableLoader: FC = ({ canCheckWorkspaces }) => { - + + + +
+ +
@@ -399,3 +441,188 @@ const WorkspaceStatusCell: FC = ({ workspace }) => { ); }; + +type WorkspaceActionsCellProps = { + workspace: Workspace; + onActionSuccess: () => Promise; + onActionError: (error: unknown) => void; +}; + +const WorkspaceActionsCell: FC = ({ + workspace, + onActionSuccess, + onActionError, +}) => { + const { user } = useAuthenticated(); + + const queryClient = useQueryClient(); + const abilities = abilitiesByWorkspaceStatus(workspace, { + canDebug: false, + isOwner: user.roles.find((role) => role.name === "owner") !== undefined, + }); + + const startWorkspaceOptions = startWorkspace(workspace, queryClient); + const startWorkspaceMutation = useMutation({ + ...startWorkspaceOptions, + onSuccess: async (build) => { + startWorkspaceOptions.onSuccess(build); + await onActionSuccess(); + }, + onError: onActionError, + }); + + const stopWorkspaceOptions = stopWorkspace(workspace, queryClient); + const stopWorkspaceMutation = useMutation({ + ...stopWorkspaceOptions, + onSuccess: async (build) => { + stopWorkspaceOptions.onSuccess(build); + await onActionSuccess(); + }, + onError: onActionError, + }); + + const cancelJobOptions = cancelBuild(workspace, queryClient); + const cancelBuildMutation = useMutation({ + ...cancelJobOptions, + onSuccess: async () => { + cancelJobOptions.onSuccess(); + await onActionSuccess(); + }, + onError: onActionError, + }); + + const { data: latestVersion } = useQuery({ + ...templateVersion(workspace.template_active_version_id), + enabled: workspace.outdated, + }); + const workspaceUpdate = useWorkspaceUpdate({ + workspace, + latestVersion, + onSuccess: onActionSuccess, + onError: onActionError, + }); + + const deleteWorkspaceOptions = deleteWorkspace(workspace, queryClient); + const deleteWorkspaceMutation = useMutation({ + ...deleteWorkspaceOptions, + onSuccess: async (build) => { + deleteWorkspaceOptions.onSuccess(build); + await onActionSuccess(); + }, + onError: onActionError, + }); + + const isRetrying = + startWorkspaceMutation.isLoading || + stopWorkspaceMutation.isLoading || + deleteWorkspaceMutation.isLoading; + + const retry = () => { + switch (workspace.latest_build.transition) { + case "start": + startWorkspaceMutation.mutate({}); + break; + case "stop": + stopWorkspaceMutation.mutate({}); + break; + case "delete": + deleteWorkspaceMutation.mutate({}); + break; + } + }; + + return ( + +
+ {abilities.actions.includes("start") && ( + startWorkspaceMutation.mutate({})} + isLoading={startWorkspaceMutation.isLoading} + label="Start workspace" + > + + + )} + + {abilities.actions.includes("updateAndStart") && ( + <> + + + + + + )} + + {abilities.actions.includes("stop") && ( + { + stopWorkspaceMutation.mutate({}); + }} + isLoading={stopWorkspaceMutation.isLoading} + label="Stop workspace" + > + + + )} + + {abilities.canCancel && ( + + + + )} + + {abilities.actions.includes("retry") && ( + + + + )} +
+ + ); +}; + +type PrimaryActionProps = PropsWithChildren<{ + onClick: () => void; + isLoading: boolean; + label: string; +}>; + +const PrimaryAction: FC = ({ + onClick, + isLoading, + label, + children, +}) => { + return ( + + + + + + {label} + + + ); +}; diff --git a/site/src/pages/WorkspacesPage/data.ts b/site/src/pages/WorkspacesPage/data.ts index 9b46ab7fed05b..764ea218aa96c 100644 --- a/site/src/pages/WorkspacesPage/data.ts +++ b/site/src/pages/WorkspacesPage/data.ts @@ -1,8 +1,10 @@ import { API } from "api/api"; import { getErrorMessage } from "api/errors"; +import { workspaces } from "api/queries/workspaces"; import type { Workspace, WorkspaceBuild, + WorkspacesRequest, WorkspacesResponse, } from "api/typesGenerated"; import { displayError } from "components/GlobalSnackbar/utils"; @@ -14,27 +16,11 @@ import { useQueryClient, } from "react-query"; -type UseWorkspacesDataParams = { - page: number; - limit: number; - query: string; -}; - -export const useWorkspacesData = ({ - page, - limit, - query, -}: UseWorkspacesDataParams) => { - const queryKey = ["workspaces", query, page]; +export const useWorkspacesData = (req: WorkspacesRequest) => { const [shouldRefetch, setShouldRefetch] = useState(true); + const workspacesQueryOptions = workspaces(req); const result = useQuery({ - queryKey, - queryFn: () => - API.getWorkspaces({ - q: query, - limit: limit, - offset: page <= 0 ? 0 : (page - 1) * limit, - }), + ...workspacesQueryOptions, onSuccess: () => { setShouldRefetch(true); }, @@ -46,7 +32,7 @@ export const useWorkspacesData = ({ return { ...result, - queryKey, + queryKey: workspacesQueryOptions.queryKey, }; }; From a7e828593f7afc19e4e0d50ab2a0918798cab772 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Tue, 6 May 2025 16:53:26 +0200 Subject: [PATCH 3/5] chore: retry failing tests in CI (#17681) This PR introduces failing test retries in CI for e2e tests, Go tests with the in-memory database, Go tests with Postgres, and the CLI tests. Retries are not enabled for race tests. The goal is to reduce how often flakes disrupt developers' workflows. --- .github/workflows/ci.yaml | 9 +++++++-- Makefile | 13 ++++++++++--- site/e2e/playwright.config.ts | 18 ++++++++++++++++++ 3 files changed, 35 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 625e6a82673e1..e46aa7eb7383a 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -382,8 +382,8 @@ jobs: touch ~/.bash_profile && echo "export BASH_SILENCE_DEPRECATION_WARNING=1" >> ~/.bash_profile fi export TS_DEBUG_DISCO=true - gotestsum --junitfile="gotests.xml" --jsonfile="gotests.json" \ - --packages="./..." -- $PARALLEL_FLAG -short -failfast + gotestsum --junitfile="gotests.xml" --jsonfile="gotests.json" --rerun-fails=2 \ + --packages="./..." -- $PARALLEL_FLAG -short - name: Upload Test Cache uses: ./.github/actions/test-cache/upload @@ -436,6 +436,7 @@ jobs: TS_DEBUG_DISCO: "true" LC_CTYPE: "en_US.UTF-8" LC_ALL: "en_US.UTF-8" + TEST_RETRIES: 2 shell: bash run: | # By default Go will use the number of logical CPUs, which @@ -499,6 +500,7 @@ jobs: TS_DEBUG_DISCO: "true" LC_CTYPE: "en_US.UTF-8" LC_ALL: "en_US.UTF-8" + TEST_RETRIES: 2 shell: bash run: | # By default Go will use the number of logical CPUs, which @@ -560,6 +562,7 @@ jobs: env: POSTGRES_VERSION: "16" TS_DEBUG_DISCO: "true" + TEST_RETRIES: 2 run: | make test-postgres @@ -784,6 +787,7 @@ jobs: if: ${{ !matrix.variant.premium }} env: DEBUG: pw:api + CODER_E2E_TEST_RETRIES: 2 working-directory: site # Run all of the tests with a premium license @@ -793,6 +797,7 @@ jobs: DEBUG: pw:api CODER_E2E_LICENSE: ${{ secrets.CODER_E2E_LICENSE }} CODER_E2E_REQUIRE_PREMIUM_TESTS: "1" + CODER_E2E_TEST_RETRIES: 2 working-directory: site - name: Upload Playwright Failed Tests diff --git a/Makefile b/Makefile index f96c8ab957442..0b8cefbab0663 100644 --- a/Makefile +++ b/Makefile @@ -875,12 +875,19 @@ provisioner/terraform/testdata/version: fi .PHONY: provisioner/terraform/testdata/version +# Set the retry flags if TEST_RETRIES is set +ifdef TEST_RETRIES +GOTESTSUM_RETRY_FLAGS := --rerun-fails=$(TEST_RETRIES) +else +GOTESTSUM_RETRY_FLAGS := +endif + test: - $(GIT_FLAGS) gotestsum --format standard-quiet -- -v -short -count=1 ./... $(if $(RUN),-run $(RUN)) + $(GIT_FLAGS) gotestsum --format standard-quiet $(GOTESTSUM_RETRY_FLAGS) --packages="./..." -- -v -short -count=1 $(if $(RUN),-run $(RUN)) .PHONY: test test-cli: - $(GIT_FLAGS) gotestsum --format standard-quiet -- -v -short -count=1 ./cli/... + $(GIT_FLAGS) gotestsum --format standard-quiet $(GOTESTSUM_RETRY_FLAGS) --packages="./cli/..." -- -v -short -count=1 .PHONY: test-cli # sqlc-cloud-is-setup will fail if no SQLc auth token is set. Use this as a @@ -919,9 +926,9 @@ test-postgres: test-postgres-docker $(GIT_FLAGS) DB=ci gotestsum \ --junitfile="gotests.xml" \ --jsonfile="gotests.json" \ + $(GOTESTSUM_RETRY_FLAGS) \ --packages="./..." -- \ -timeout=20m \ - -failfast \ -count=1 .PHONY: test-postgres diff --git a/site/e2e/playwright.config.ts b/site/e2e/playwright.config.ts index 762b7f0158dba..436af99240493 100644 --- a/site/e2e/playwright.config.ts +++ b/site/e2e/playwright.config.ts @@ -10,12 +10,30 @@ import { } from "./constants"; export const wsEndpoint = process.env.CODER_E2E_WS_ENDPOINT; +export const retries = (() => { + if (process.env.CODER_E2E_TEST_RETRIES === undefined) { + return undefined; + } + const count = Number.parseInt(process.env.CODER_E2E_TEST_RETRIES, 10); + if (Number.isNaN(count)) { + throw new Error( + `CODER_E2E_TEST_RETRIES is not a number: ${process.env.CODER_E2E_TEST_RETRIES}`, + ); + } + if (count < 0) { + throw new Error( + `CODER_E2E_TEST_RETRIES is less than 0: ${process.env.CODER_E2E_TEST_RETRIES}`, + ); + } + return count; +})(); const localURL = (port: number, path: string): string => { return `http://localhost:${port}${path}`; }; export default defineConfig({ + retries, globalSetup: require.resolve("./setup/preflight"), projects: [ { From 4fa9d30bf4b70f73264b81ad6acd773d3eb00166 Mon Sep 17 00:00:00 2001 From: Bruno Quaresma Date: Tue, 6 May 2025 13:26:37 -0300 Subject: [PATCH 4/5] refactor: update app buttons to use the new button component (#17684) Related to https://github.com/coder/coder/issues/17311 - Replaces the MUI Buttons by the new shadcn/ui buttons. This change allows the reuse of app links, and terminal buttons using the `asChild` capability from the Radix components - Uses the new [proposed design](https://www.figma.com/design/OR75XeUI0Z3ksqt1mHsNQw/Workspace-views?node-id=1014-8242&t=wtUXJRN1SfyZiFKn-0) - Updates the button styles to support image tags as icons - Uses the new Tooltip component for the app buttons **Before:** Screenshot 2025-05-05 at 17 55 49 **After:** Screenshot 2025-05-05 at 18 05 38 --- site/e2e/helpers.ts | 4 +- site/src/components/Button/Button.tsx | 12 ++-- .../ExternalImage/ExternalImage.tsx | 8 +-- .../modules/dashboard/Navbar/ProxyMenu.tsx | 25 +++---- .../management/OrganizationSidebarView.tsx | 34 +++++---- site/src/modules/resources/AgentButton.tsx | 27 +------ .../resources/AgentDevcontainerCard.tsx | 38 +++++----- .../src/modules/resources/AppLink/AppLink.tsx | 70 +++++++++---------- .../resources/TerminalLink/TerminalLink.tsx | 35 +++++----- .../VSCodeDesktopButton.tsx | 20 +++--- .../VSCodeDevContainerButton.tsx | 20 +++--- site/src/theme/externalImages.ts | 2 - 12 files changed, 128 insertions(+), 167 deletions(-) diff --git a/site/e2e/helpers.ts b/site/e2e/helpers.ts index 71b1c039c5dfb..85a9283abae04 100644 --- a/site/e2e/helpers.ts +++ b/site/e2e/helpers.ts @@ -1042,7 +1042,9 @@ export async function openTerminalWindow( ): Promise { // Wait for the web terminal to open in a new tab const pagePromise = context.waitForEvent("page"); - await page.getByTestId("terminal").click({ timeout: 60_000 }); + await page + .getByRole("link", { name: /terminal/i }) + .click({ timeout: 60_000 }); const terminal = await pagePromise; await terminal.waitForLoadState("domcontentloaded"); diff --git a/site/src/components/Button/Button.tsx b/site/src/components/Button/Button.tsx index f3940fe45cabc..908dacb8c5c3d 100644 --- a/site/src/components/Button/Button.tsx +++ b/site/src/components/Button/Button.tsx @@ -13,7 +13,9 @@ const buttonVariants = cva( text-sm font-semibold font-medium cursor-pointer no-underline focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-content-link disabled:pointer-events-none disabled:text-content-disabled - [&_svg]:pointer-events-none [&_svg]:shrink-0 [&_svg]:p-0.5`, + [&:is(a):not([href])]:pointer-events-none [&:is(a):not([href])]:text-content-disabled + [&_svg]:pointer-events-none [&_svg]:shrink-0 [&_svg]:p-0.5 + [&>img]:pointer-events-none [&>img]:shrink-0 [&>img]:p-0.5`, { variants: { variant: { @@ -28,11 +30,11 @@ const buttonVariants = cva( }, size: { - lg: "min-w-20 h-10 px-3 py-2 [&_svg]:size-icon-lg", - sm: "min-w-20 h-8 px-2 py-1.5 text-xs [&_svg]:size-icon-sm", + lg: "min-w-20 h-10 px-3 py-2 [&_svg]:size-icon-lg [&>img]:size-icon-lg", + sm: "min-w-20 h-8 px-2 py-1.5 text-xs [&_svg]:size-icon-sm [&>img]:size-icon-sm", xs: "min-w-8 py-1 px-2 text-2xs rounded-md", - icon: "size-8 px-1.5 [&_svg]:size-icon-sm", - "icon-lg": "size-10 px-2 [&_svg]:size-icon-lg", + icon: "size-8 px-1.5 [&_svg]:size-icon-sm [&>img]:size-icon-sm", + "icon-lg": "size-10 px-2 [&_svg]:size-icon-lg [&>img]:size-icon-lg", }, }, defaultVariants: { diff --git a/site/src/components/ExternalImage/ExternalImage.tsx b/site/src/components/ExternalImage/ExternalImage.tsx index b15af07944ca8..d85b227b999b4 100644 --- a/site/src/components/ExternalImage/ExternalImage.tsx +++ b/site/src/components/ExternalImage/ExternalImage.tsx @@ -5,15 +5,15 @@ import { getExternalImageStylesFromUrl } from "theme/externalImages"; export const ExternalImage = forwardRef< HTMLImageElement, ImgHTMLAttributes ->((attrs, ref) => { +>((props, ref) => { const theme = useTheme(); return ( - // biome-ignore lint/a11y/useAltText: no reasonable alt to provide + // biome-ignore lint/a11y/useAltText: alt should be passed in as a prop ); }); diff --git a/site/src/modules/dashboard/Navbar/ProxyMenu.tsx b/site/src/modules/dashboard/Navbar/ProxyMenu.tsx index 86d9b9b53ee84..97e360984357f 100644 --- a/site/src/modules/dashboard/Navbar/ProxyMenu.tsx +++ b/site/src/modules/dashboard/Navbar/ProxyMenu.tsx @@ -81,32 +81,25 @@ export const ProxyMenu: FC = ({ proxyContextValue }) => { {selectedProxy ? ( -
-
- -
+ <> + -
+ ) : ( "Select Proxy" )} - + -
- {activeOrganization ? ( - <> - - - {activeOrganization.display_name || activeOrganization.name} - - - ) : ( - No organization selected - )} -
- + {activeOrganization ? ( + <> + + + {activeOrganization.display_name || activeOrganization.name} + + + ) : ( + No organization selected + )} + diff --git a/site/src/modules/resources/AgentButton.tsx b/site/src/modules/resources/AgentButton.tsx index 2f772e4f8e0ca..e5b4a54834531 100644 --- a/site/src/modules/resources/AgentButton.tsx +++ b/site/src/modules/resources/AgentButton.tsx @@ -1,31 +1,8 @@ -import Button, { type ButtonProps } from "@mui/material/Button"; +import { Button, type ButtonProps } from "components/Button/Button"; import { forwardRef } from "react"; export const AgentButton = forwardRef( (props, ref) => { - const { children, ...buttonProps } = props; - - return ( - - ); + return
diff --git a/site/src/modules/resources/AppLink/AppLink.tsx b/site/src/modules/resources/AppLink/AppLink.tsx index 5bf2114acf879..8eeef61ee920e 100644 --- a/site/src/modules/resources/AppLink/AppLink.tsx +++ b/site/src/modules/resources/AppLink/AppLink.tsx @@ -1,13 +1,17 @@ import { useTheme } from "@emotion/react"; import ErrorOutlineIcon from "@mui/icons-material/ErrorOutline"; -import CircularProgress from "@mui/material/CircularProgress"; -import Link from "@mui/material/Link"; -import Tooltip from "@mui/material/Tooltip"; import { API } from "api/api"; import type * as TypesGen from "api/typesGenerated"; import { displayError } from "components/GlobalSnackbar/utils"; +import { Spinner } from "components/Spinner/Spinner"; +import { + Tooltip, + TooltipContent, + TooltipProvider, + TooltipTrigger, +} from "components/Tooltip/Tooltip"; import { useProxy } from "contexts/ProxyContext"; -import { type FC, type MouseEvent, useState } from "react"; +import { type FC, useState } from "react"; import { createAppLinkHref } from "utils/apps"; import { generateRandomString } from "utils/random"; import { AgentButton } from "../AgentButton"; @@ -75,21 +79,7 @@ export const AppLink: FC = ({ app, workspace, agent }) => { let primaryTooltip = ""; if (app.health === "initializing") { - icon = ( - // This is a hack to make the spinner appear in the center of the start - // icon space - - - - ); + icon = ; primaryTooltip = "Initializing..."; } if (app.health === "unhealthy") { @@ -112,22 +102,13 @@ export const AppLink: FC = ({ app, workspace, agent }) => { canClick = false; } - const isPrivateApp = app.sharing_level === "owner"; - - return ( - - } - disabled={!canClick} - href={href} - css={{ - pointerEvents: canClick ? undefined : "none", - textDecoration: "none !important", - }} - onClick={async (event: MouseEvent) => { + const canShare = app.sharing_level !== "owner"; + + const button = ( + + { if (!canClick) { return; } @@ -187,8 +168,23 @@ export const AppLink: FC = ({ app, workspace, agent }) => { } }} > + {icon} {appDisplayName} - - + {canShare && } + + ); + + if (primaryTooltip) { + return ( + + + {button} + {primaryTooltip} + + + ); + } + + return button; }; diff --git a/site/src/modules/resources/TerminalLink/TerminalLink.tsx b/site/src/modules/resources/TerminalLink/TerminalLink.tsx index 6a4e6d41f3ffc..23204bd819dfe 100644 --- a/site/src/modules/resources/TerminalLink/TerminalLink.tsx +++ b/site/src/modules/resources/TerminalLink/TerminalLink.tsx @@ -1,4 +1,3 @@ -import Link from "@mui/material/Link"; import { TerminalIcon } from "components/Icons/TerminalIcon"; import type { FC, MouseEvent } from "react"; import { generateRandomString } from "utils/random"; @@ -39,23 +38,21 @@ export const TerminalLink: FC = ({ }/terminal?${params.toString()}`; return ( - } - href={href} - onClick={(event: MouseEvent) => { - event.preventDefault(); - window.open( - href, - Language.terminalTitle(generateRandomString(12)), - "width=900,height=600", - ); - }} - data-testid="terminal" - > - {DisplayAppNameMap.web_terminal} - + + ) => { + event.preventDefault(); + window.open( + href, + Language.terminalTitle(generateRandomString(12)), + "width=900,height=600", + ); + }} + > + + {DisplayAppNameMap.web_terminal} + + ); }; diff --git a/site/src/modules/resources/VSCodeDesktopButton/VSCodeDesktopButton.tsx b/site/src/modules/resources/VSCodeDesktopButton/VSCodeDesktopButton.tsx index 10193660155eb..8397305ef153f 100644 --- a/site/src/modules/resources/VSCodeDesktopButton/VSCodeDesktopButton.tsx +++ b/site/src/modules/resources/VSCodeDesktopButton/VSCodeDesktopButton.tsx @@ -1,11 +1,10 @@ -import KeyboardArrowDownIcon from "@mui/icons-material/KeyboardArrowDown"; -import ButtonGroup from "@mui/material/ButtonGroup"; import Menu from "@mui/material/Menu"; import MenuItem from "@mui/material/MenuItem"; import { API } from "api/api"; import type { DisplayApp } from "api/typesGenerated"; import { VSCodeIcon } from "components/Icons/VSCodeIcon"; import { VSCodeInsidersIcon } from "components/Icons/VSCodeInsidersIcon"; +import { ChevronDownIcon } from "lucide-react"; import { type FC, useRef, useState } from "react"; import { AgentButton } from "../AgentButton"; import { DisplayAppNameMap } from "../AppLink/AppLink"; @@ -43,8 +42,8 @@ export const VSCodeDesktopButton: FC = (props) => { const includesVSCodeInsiders = props.displayApps.includes("vscode_insiders"); return includesVSCodeDesktop && includesVSCodeInsiders ? ( -
- + <> +
{variant === "vscode" ? ( ) : ( @@ -58,15 +57,14 @@ export const VSCodeDesktopButton: FC = (props) => { aria-expanded={isVariantMenuOpen ? "true" : undefined} aria-label="select VSCode variant" aria-haspopup="menu" - disableRipple onClick={() => { setIsVariantMenuOpen(true); }} - css={{ paddingLeft: 0, paddingRight: 0 }} + size="icon-lg" > - + - +
= (props) => { {DisplayAppNameMap.vscode_insiders} -
+ ) : includesVSCodeDesktop ? ( ) : ( @@ -115,7 +113,6 @@ const VSCodeButton: FC = ({ return ( } disabled={loading} onClick={() => { setLoading(true); @@ -145,6 +142,7 @@ const VSCodeButton: FC = ({ }); }} > + {DisplayAppNameMap.vscode} ); @@ -160,7 +158,6 @@ const VSCodeInsidersButton: FC = ({ return ( } disabled={loading} onClick={() => { setLoading(true); @@ -189,6 +186,7 @@ const VSCodeInsidersButton: FC = ({ }); }} > + {DisplayAppNameMap.vscode_insiders} ); diff --git a/site/src/modules/resources/VSCodeDevContainerButton/VSCodeDevContainerButton.tsx b/site/src/modules/resources/VSCodeDevContainerButton/VSCodeDevContainerButton.tsx index 3b32c672e8e8f..cbd5aba4efa90 100644 --- a/site/src/modules/resources/VSCodeDevContainerButton/VSCodeDevContainerButton.tsx +++ b/site/src/modules/resources/VSCodeDevContainerButton/VSCodeDevContainerButton.tsx @@ -1,11 +1,10 @@ -import KeyboardArrowDownIcon from "@mui/icons-material/KeyboardArrowDown"; -import ButtonGroup from "@mui/material/ButtonGroup"; import Menu from "@mui/material/Menu"; import MenuItem from "@mui/material/MenuItem"; import { API } from "api/api"; import type { DisplayApp } from "api/typesGenerated"; import { VSCodeIcon } from "components/Icons/VSCodeIcon"; import { VSCodeInsidersIcon } from "components/Icons/VSCodeInsidersIcon"; +import { ChevronDownIcon } from "lucide-react"; import { type FC, useRef, useState } from "react"; import { AgentButton } from "../AgentButton"; import { DisplayAppNameMap } from "../AppLink/AppLink"; @@ -46,8 +45,8 @@ export const VSCodeDevContainerButton: FC = ( const includesVSCodeInsiders = props.displayApps.includes("vscode_insiders"); return includesVSCodeDesktop && includesVSCodeInsiders ? ( -
- + <> +
{variant === "vscode" ? ( ) : ( @@ -61,15 +60,14 @@ export const VSCodeDevContainerButton: FC = ( aria-expanded={isVariantMenuOpen ? "true" : undefined} aria-label="select VSCode variant" aria-haspopup="menu" - disableRipple onClick={() => { setIsVariantMenuOpen(true); }} - css={{ paddingLeft: 0, paddingRight: 0 }} + size="icon-lg" > - + - +
= ( {DisplayAppNameMap.vscode_insiders} -
+ ) : includesVSCodeDesktop ? ( ) : ( @@ -119,7 +117,6 @@ const VSCodeButton: FC = ({ return ( } disabled={loading} onClick={() => { setLoading(true); @@ -147,6 +144,7 @@ const VSCodeButton: FC = ({ }); }} > + {DisplayAppNameMap.vscode} ); @@ -163,7 +161,6 @@ const VSCodeInsidersButton: FC = ({ return ( } disabled={loading} onClick={() => { setLoading(true); @@ -191,6 +188,7 @@ const VSCodeInsidersButton: FC = ({ }); }} > + {DisplayAppNameMap.vscode_insiders} ); diff --git a/site/src/theme/externalImages.ts b/site/src/theme/externalImages.ts index 889347ea0ec74..22c94b9a44b4b 100644 --- a/site/src/theme/externalImages.ts +++ b/site/src/theme/externalImages.ts @@ -1,7 +1,5 @@ import type { CSSObject } from "@emotion/react"; -type ExternalImageMode = keyof ExternalImageModeStyles; - export interface ExternalImageModeStyles { /** * monochrome icons will be flattened to a neutral, theme-appropriate color. From df0c6eda33e9db32ac1af10de37dccc0f16658c0 Mon Sep 17 00:00:00 2001 From: DevCats Date: Tue, 6 May 2025 11:46:36 -0500 Subject: [PATCH 5/5] chore: add custom aider icon (#17682) Created Custom SVG from Aider PNG and upload from module to static site icons --- site/src/theme/icons.json | 1 + site/static/icon/aider.svg | 3 +++ 2 files changed, 4 insertions(+) create mode 100644 site/static/icon/aider.svg diff --git a/site/src/theme/icons.json b/site/src/theme/icons.json index a9307bfc78446..9dcc10321682c 100644 --- a/site/src/theme/icons.json +++ b/site/src/theme/icons.json @@ -1,4 +1,5 @@ [ + "aider.svg", "almalinux.svg", "android-studio.svg", "apache-guacamole.svg", diff --git a/site/static/icon/aider.svg b/site/static/icon/aider.svg new file mode 100644 index 0000000000000..44e064ff3abb4 --- /dev/null +++ b/site/static/icon/aider.svg @@ -0,0 +1,3 @@ + + + \ No newline at end of file