mirror of
https://github.com/coder/coder.git
synced 2026-09-22 05:05:20 +08:00
fix: populate all chat fields in pubsub events (#23664)
*Problem:* `publishChatPubsubEvent` was constructing a partial
`codersdk.Chat` that omitted `LastModelConfigID` and other fields. Go's
zero-value UUID caused the sidebar to show "Default model" for chats
received via SSE.
*Solution:*
- Extracted `convertChat`/`convertChats` from `exp_chats.go` into
`db2sdk.Chat`/`db2sdk.Chats`, alongside existing `ChatMessage`,
`ChatQueuedMessage`, and `ChatDiffStatus` converters.
`publishChatPubsubEvent` now calls `db2sdk.Chat(chat, nil)` instead of
maintaining its own copy of the conversion logic
- Added backend integration test
`TestWatchChats/CreatedEventIncludesAllChatFields`
- Added frontend regression tests for nil-UUID and valid model config ID
cases
> 🤖 Created by Coder Agents, reviewed by this human.
This commit is contained in:
@@ -1516,6 +1516,85 @@ func nullInt64Ptr(v sql.NullInt64) *int64 {
|
||||
return &value
|
||||
}
|
||||
|
||||
// Chat converts a database.Chat to a codersdk.Chat. It coalesces
|
||||
// nil slices and maps to empty values for JSON serialization and
|
||||
// derives RootChatID from the parent chain when not explicitly set.
|
||||
func Chat(c database.Chat, diffStatus *database.ChatDiffStatus) codersdk.Chat {
|
||||
mcpServerIDs := c.MCPServerIDs
|
||||
if mcpServerIDs == nil {
|
||||
mcpServerIDs = []uuid.UUID{}
|
||||
}
|
||||
labels := map[string]string(c.Labels)
|
||||
if labels == nil {
|
||||
labels = map[string]string{}
|
||||
}
|
||||
chat := codersdk.Chat{
|
||||
ID: c.ID,
|
||||
OwnerID: c.OwnerID,
|
||||
LastModelConfigID: c.LastModelConfigID,
|
||||
Title: c.Title,
|
||||
Status: codersdk.ChatStatus(c.Status),
|
||||
Archived: c.Archived,
|
||||
CreatedAt: c.CreatedAt,
|
||||
UpdatedAt: c.UpdatedAt,
|
||||
MCPServerIDs: mcpServerIDs,
|
||||
Labels: labels,
|
||||
}
|
||||
if c.LastError.Valid {
|
||||
chat.LastError = &c.LastError.String
|
||||
}
|
||||
if c.ParentChatID.Valid {
|
||||
parentChatID := c.ParentChatID.UUID
|
||||
chat.ParentChatID = &parentChatID
|
||||
}
|
||||
switch {
|
||||
case c.RootChatID.Valid:
|
||||
rootChatID := c.RootChatID.UUID
|
||||
chat.RootChatID = &rootChatID
|
||||
case c.ParentChatID.Valid:
|
||||
rootChatID := c.ParentChatID.UUID
|
||||
chat.RootChatID = &rootChatID
|
||||
default:
|
||||
rootChatID := c.ID
|
||||
chat.RootChatID = &rootChatID
|
||||
}
|
||||
if c.WorkspaceID.Valid {
|
||||
chat.WorkspaceID = &c.WorkspaceID.UUID
|
||||
}
|
||||
if c.BuildID.Valid {
|
||||
chat.BuildID = &c.BuildID.UUID
|
||||
}
|
||||
if c.AgentID.Valid {
|
||||
chat.AgentID = &c.AgentID.UUID
|
||||
}
|
||||
if diffStatus != nil {
|
||||
convertedDiffStatus := ChatDiffStatus(c.ID, diffStatus)
|
||||
chat.DiffStatus = &convertedDiffStatus
|
||||
}
|
||||
return chat
|
||||
}
|
||||
|
||||
// Chats converts a slice of database.Chat to codersdk.Chat, looking
|
||||
// up diff statuses from the provided map. When diffStatusesByChatID
|
||||
// is non-nil, chats without an entry receive an empty DiffStatus.
|
||||
func Chats(chats []database.Chat, diffStatusesByChatID map[uuid.UUID]database.ChatDiffStatus) []codersdk.Chat {
|
||||
result := make([]codersdk.Chat, len(chats))
|
||||
for i, c := range chats {
|
||||
diffStatus, ok := diffStatusesByChatID[c.ID]
|
||||
if ok {
|
||||
result[i] = Chat(c, &diffStatus)
|
||||
continue
|
||||
}
|
||||
|
||||
result[i] = Chat(c, nil)
|
||||
if diffStatusesByChatID != nil {
|
||||
emptyDiffStatus := ChatDiffStatus(c.ID, nil)
|
||||
result[i].DiffStatus = &emptyDiffStatus
|
||||
}
|
||||
}
|
||||
return result
|
||||
}
|
||||
|
||||
// ChatDiffStatus converts a database.ChatDiffStatus to a
|
||||
// codersdk.ChatDiffStatus. When status is nil an empty value
|
||||
// containing only the chatID is returned.
|
||||
|
||||
@@ -5,6 +5,7 @@ import (
|
||||
"database/sql"
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"reflect"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
@@ -513,6 +514,54 @@ func TestChatQueuedMessage_ParsesUserContentParts(t *testing.T) {
|
||||
require.Equal(t, "queued text", queued.Content[0].Text)
|
||||
}
|
||||
|
||||
func TestChat_AllFieldsPopulated(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
// Every field of database.Chat is set to a non-zero value so
|
||||
// that the reflection check below catches any field that
|
||||
// db2sdk.Chat forgets to populate. When someone adds a new
|
||||
// field to codersdk.Chat, this test will fail until the
|
||||
// converter is updated.
|
||||
now := dbtime.Now()
|
||||
input := database.Chat{
|
||||
ID: uuid.New(),
|
||||
OwnerID: uuid.New(),
|
||||
WorkspaceID: uuid.NullUUID{UUID: uuid.New(), Valid: true},
|
||||
BuildID: uuid.NullUUID{UUID: uuid.New(), Valid: true},
|
||||
AgentID: uuid.NullUUID{UUID: uuid.New(), Valid: true},
|
||||
ParentChatID: uuid.NullUUID{UUID: uuid.New(), Valid: true},
|
||||
RootChatID: uuid.NullUUID{UUID: uuid.New(), Valid: true},
|
||||
LastModelConfigID: uuid.New(),
|
||||
Title: "all-fields-test",
|
||||
Status: database.ChatStatusRunning,
|
||||
LastError: sql.NullString{String: "boom", Valid: true},
|
||||
CreatedAt: now,
|
||||
UpdatedAt: now,
|
||||
Archived: true,
|
||||
MCPServerIDs: []uuid.UUID{uuid.New()},
|
||||
Labels: database.StringMap{"env": "prod"},
|
||||
}
|
||||
// Only ChatID is needed here. This test checks that
|
||||
// Chat.DiffStatus is non-nil, not that every DiffStatus
|
||||
// field is populated — that would be a separate test for
|
||||
// the ChatDiffStatus converter.
|
||||
diffStatus := &database.ChatDiffStatus{
|
||||
ChatID: input.ID,
|
||||
}
|
||||
|
||||
got := db2sdk.Chat(input, diffStatus)
|
||||
|
||||
v := reflect.ValueOf(got)
|
||||
typ := v.Type()
|
||||
for i := range typ.NumField() {
|
||||
field := typ.Field(i)
|
||||
require.False(t, v.Field(i).IsZero(),
|
||||
"codersdk.Chat field %q is zero-valued — db2sdk.Chat may not be populating it",
|
||||
field.Name,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
func TestChatQueuedMessage_MalformedContent(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
|
||||
+4
-77
@@ -354,7 +354,7 @@ func (api *API) listChats(rw http.ResponseWriter, r *http.Request) {
|
||||
return
|
||||
}
|
||||
|
||||
httpapi.Write(ctx, rw, http.StatusOK, convertChats(chats, diffStatusesByChatID))
|
||||
httpapi.Write(ctx, rw, http.StatusOK, db2sdk.Chats(chats, diffStatusesByChatID))
|
||||
}
|
||||
|
||||
func (api *API) getChatDiffStatusesByChatID(
|
||||
@@ -499,7 +499,7 @@ func (api *API) postChats(rw http.ResponseWriter, r *http.Request) {
|
||||
return
|
||||
}
|
||||
|
||||
httpapi.Write(ctx, rw, http.StatusCreated, convertChat(chat, nil))
|
||||
httpapi.Write(ctx, rw, http.StatusCreated, db2sdk.Chat(chat, nil))
|
||||
}
|
||||
|
||||
// EXPERIMENTAL: this endpoint is experimental and is subject to change.
|
||||
@@ -1224,7 +1224,7 @@ func (api *API) getChat(rw http.ResponseWriter, r *http.Request) {
|
||||
slog.Error(err),
|
||||
)
|
||||
}
|
||||
httpapi.Write(ctx, rw, http.StatusOK, convertChat(chat, diffStatus))
|
||||
httpapi.Write(ctx, rw, http.StatusOK, db2sdk.Chat(chat, diffStatus))
|
||||
}
|
||||
|
||||
// EXPERIMENTAL: this endpoint is experimental and is subject to change.
|
||||
@@ -2054,7 +2054,7 @@ func (api *API) interruptChat(rw http.ResponseWriter, r *http.Request) {
|
||||
chat = updatedChat
|
||||
}
|
||||
|
||||
httpapi.Write(ctx, rw, http.StatusOK, convertChat(chat, nil))
|
||||
httpapi.Write(ctx, rw, http.StatusOK, db2sdk.Chat(chat, nil))
|
||||
}
|
||||
|
||||
// EXPERIMENTAL: this endpoint is experimental and is subject to change.
|
||||
@@ -3696,79 +3696,6 @@ func truncateRunes(value string, maxLen int) string {
|
||||
return string(runes[:maxLen])
|
||||
}
|
||||
|
||||
func convertChat(c database.Chat, diffStatus *database.ChatDiffStatus) codersdk.Chat {
|
||||
mcpServerIDs := c.MCPServerIDs
|
||||
if mcpServerIDs == nil {
|
||||
mcpServerIDs = []uuid.UUID{}
|
||||
}
|
||||
labels := map[string]string(c.Labels)
|
||||
if labels == nil {
|
||||
labels = map[string]string{}
|
||||
}
|
||||
chat := codersdk.Chat{
|
||||
ID: c.ID,
|
||||
OwnerID: c.OwnerID,
|
||||
LastModelConfigID: c.LastModelConfigID,
|
||||
Title: c.Title,
|
||||
Status: codersdk.ChatStatus(c.Status),
|
||||
Archived: c.Archived,
|
||||
CreatedAt: c.CreatedAt,
|
||||
UpdatedAt: c.UpdatedAt,
|
||||
MCPServerIDs: mcpServerIDs,
|
||||
Labels: labels,
|
||||
}
|
||||
if c.LastError.Valid {
|
||||
chat.LastError = &c.LastError.String
|
||||
}
|
||||
if c.ParentChatID.Valid {
|
||||
parentChatID := c.ParentChatID.UUID
|
||||
chat.ParentChatID = &parentChatID
|
||||
}
|
||||
switch {
|
||||
case c.RootChatID.Valid:
|
||||
rootChatID := c.RootChatID.UUID
|
||||
chat.RootChatID = &rootChatID
|
||||
case c.ParentChatID.Valid:
|
||||
rootChatID := c.ParentChatID.UUID
|
||||
chat.RootChatID = &rootChatID
|
||||
default:
|
||||
rootChatID := c.ID
|
||||
chat.RootChatID = &rootChatID
|
||||
}
|
||||
if c.WorkspaceID.Valid {
|
||||
chat.WorkspaceID = &c.WorkspaceID.UUID
|
||||
}
|
||||
if c.BuildID.Valid {
|
||||
chat.BuildID = &c.BuildID.UUID
|
||||
}
|
||||
if c.AgentID.Valid {
|
||||
chat.AgentID = &c.AgentID.UUID
|
||||
}
|
||||
if diffStatus != nil {
|
||||
convertedDiffStatus := db2sdk.ChatDiffStatus(c.ID, diffStatus)
|
||||
chat.DiffStatus = &convertedDiffStatus
|
||||
}
|
||||
return chat
|
||||
}
|
||||
|
||||
func convertChats(chats []database.Chat, diffStatusesByChatID map[uuid.UUID]database.ChatDiffStatus) []codersdk.Chat {
|
||||
result := make([]codersdk.Chat, len(chats))
|
||||
for i, c := range chats {
|
||||
diffStatus, ok := diffStatusesByChatID[c.ID]
|
||||
if ok {
|
||||
result[i] = convertChat(c, &diffStatus)
|
||||
continue
|
||||
}
|
||||
|
||||
result[i] = convertChat(c, nil)
|
||||
if diffStatusesByChatID != nil {
|
||||
emptyDiffStatus := db2sdk.ChatDiffStatus(c.ID, nil)
|
||||
result[i].DiffStatus = &emptyDiffStatus
|
||||
}
|
||||
}
|
||||
return result
|
||||
}
|
||||
|
||||
func convertChatCostModelBreakdown(model database.GetChatCostPerModelRow) codersdk.ChatCostModelBreakdown {
|
||||
displayName := strings.TrimSpace(model.DisplayName)
|
||||
if displayName == "" {
|
||||
|
||||
@@ -775,6 +775,79 @@ func TestWatchChats(t *testing.T) {
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("CreatedEventIncludesAllChatFields", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
// This test verifies that the pubsub "created" event
|
||||
// carries a fully-populated codersdk.Chat. Exhaustive
|
||||
// field-level coverage of the converter is handled by
|
||||
// TestChat_AllFieldsPopulated (db2sdk) and
|
||||
// TestChat_JSONRoundTrip (codersdk). This integration
|
||||
// test only checks that key fields survive the full
|
||||
// API → pubsub → websocket pipeline.
|
||||
ctx := testutil.Context(t, testutil.WaitLong)
|
||||
client := newChatClient(t)
|
||||
_ = coderdtest.CreateFirstUser(t, client.Client)
|
||||
modelConfig := createChatModelConfig(t, client)
|
||||
|
||||
conn, err := client.Dial(ctx, "/api/experimental/chats/watch", nil)
|
||||
require.NoError(t, err)
|
||||
defer conn.Close(websocket.StatusNormalClosure, "done")
|
||||
|
||||
type watchEvent struct {
|
||||
Type codersdk.ServerSentEventType `json:"type"`
|
||||
Data json.RawMessage `json:"data,omitempty"`
|
||||
}
|
||||
|
||||
// Skip the initial ping.
|
||||
var event watchEvent
|
||||
err = wsjson.Read(ctx, conn, &event)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, codersdk.ServerSentEventTypePing, event.Type)
|
||||
require.True(t, len(event.Data) == 0 || string(event.Data) == "null")
|
||||
|
||||
createdChat, err := client.CreateChat(ctx, codersdk.CreateChatRequest{
|
||||
Content: []codersdk.ChatInputPart{
|
||||
{
|
||||
Type: codersdk.ChatInputPartTypeText,
|
||||
Text: "watch route fields completeness test",
|
||||
},
|
||||
},
|
||||
})
|
||||
require.NoError(t, err)
|
||||
|
||||
var got codersdk.Chat
|
||||
testutil.Eventually(ctx, t, func(_ context.Context) bool {
|
||||
var update watchEvent
|
||||
if readErr := wsjson.Read(ctx, conn, &update); readErr != nil {
|
||||
return false
|
||||
}
|
||||
if update.Type != codersdk.ServerSentEventTypeData {
|
||||
return false
|
||||
}
|
||||
var payload coderdpubsub.ChatEvent
|
||||
if unmarshalErr := json.Unmarshal(update.Data, &payload); unmarshalErr != nil {
|
||||
return false
|
||||
}
|
||||
if payload.Kind == coderdpubsub.ChatEventKindCreated &&
|
||||
payload.Chat.ID == createdChat.ID {
|
||||
got = payload.Chat
|
||||
return true
|
||||
}
|
||||
return false
|
||||
}, testutil.IntervalFast, "expected a created event for chat %s", createdChat.ID)
|
||||
|
||||
require.Equal(t, createdChat.ID, got.ID)
|
||||
require.Equal(t, createdChat.OwnerID, got.OwnerID)
|
||||
require.Equal(t, modelConfig.ID, got.LastModelConfigID)
|
||||
require.Equal(t, createdChat.Title, got.Title)
|
||||
require.Equal(t, codersdk.ChatStatusPending, got.Status)
|
||||
require.NotNil(t, got.RootChatID)
|
||||
require.Equal(t, createdChat.ID, *got.RootChatID)
|
||||
require.NotZero(t, got.CreatedAt)
|
||||
require.NotZero(t, got.UpdatedAt)
|
||||
})
|
||||
|
||||
t.Run("DiffStatusChangeIncludesDiffStatus", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
|
||||
+1
-22
@@ -2479,28 +2479,7 @@ func (p *Server) publishChatPubsubEvent(chat database.Chat, kind coderdpubsub.Ch
|
||||
if p.pubsub == nil {
|
||||
return
|
||||
}
|
||||
sdkChat := codersdk.Chat{
|
||||
ID: chat.ID,
|
||||
OwnerID: chat.OwnerID,
|
||||
Title: chat.Title,
|
||||
Status: codersdk.ChatStatus(chat.Status),
|
||||
CreatedAt: chat.CreatedAt,
|
||||
UpdatedAt: chat.UpdatedAt,
|
||||
}
|
||||
if chat.ParentChatID.Valid {
|
||||
parentChatID := chat.ParentChatID.UUID
|
||||
sdkChat.ParentChatID = &parentChatID
|
||||
}
|
||||
if chat.RootChatID.Valid {
|
||||
rootChatID := chat.RootChatID.UUID
|
||||
sdkChat.RootChatID = &rootChatID
|
||||
} else if !chat.ParentChatID.Valid {
|
||||
rootChatID := chat.ID
|
||||
sdkChat.RootChatID = &rootChatID
|
||||
}
|
||||
if chat.WorkspaceID.Valid {
|
||||
sdkChat.WorkspaceID = &chat.WorkspaceID.UUID
|
||||
}
|
||||
sdkChat := db2sdk.Chat(chat, nil) // we have diffStatus already converted
|
||||
if diffStatus != nil {
|
||||
sdkChat.DiffStatus = diffStatus
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user