diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 0c04ae18e5..871f530641 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -3791,8 +3791,6 @@ CREATE INDEX idx_chats_last_model_config_id ON chats USING btree (last_model_con CREATE INDEX idx_chats_owner ON chats USING btree (owner_id); -CREATE INDEX idx_chats_owner_updated_id ON chats USING btree (owner_id, updated_at DESC, id DESC); - CREATE INDEX idx_chats_parent_chat_id ON chats USING btree (parent_chat_id); CREATE INDEX idx_chats_pending ON chats USING btree (status) WHERE (status = 'pending'::chat_status); diff --git a/coderd/database/migrations/000466_drop_chat_pagination_index.down.sql b/coderd/database/migrations/000466_drop_chat_pagination_index.down.sql new file mode 100644 index 0000000000..ea5aaf861b --- /dev/null +++ b/coderd/database/migrations/000466_drop_chat_pagination_index.down.sql @@ -0,0 +1 @@ +CREATE INDEX idx_chats_owner_updated_id ON chats (owner_id, updated_at DESC, id DESC); diff --git a/coderd/database/migrations/000466_drop_chat_pagination_index.up.sql b/coderd/database/migrations/000466_drop_chat_pagination_index.up.sql new file mode 100644 index 0000000000..1476677df7 --- /dev/null +++ b/coderd/database/migrations/000466_drop_chat_pagination_index.up.sql @@ -0,0 +1,5 @@ +-- The GetChats ORDER BY changed from (updated_at, id) DESC to a 4-column +-- expression sort (pinned-first flag, negated pin_order, updated_at, id). +-- This index was purpose-built for the old sort and no longer provides +-- read benefit. The simpler idx_chats_owner covers the owner_id filter. +DROP INDEX IF EXISTS idx_chats_owner_updated_id; diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 7f82407e00..b5381ffe62 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5818,20 +5818,18 @@ WHERE ELSE chats.archived = $2 :: boolean END AND CASE - -- This allows using the last element on a page as effectively a cursor. - -- This is an important option for scripts that need to paginate without - -- duplicating or missing data. + -- Cursor pagination: the last element on a page acts as the cursor. + -- The 4-tuple matches the ORDER BY below. All columns sort DESC + -- (pin_order is negated so lower values sort first in DESC order), + -- which lets us use a single tuple < comparison. WHEN $3 :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN ( - -- The pagination cursor is the last ID of the previous page. - -- The query is ordered by the updated_at field, so select all - -- rows before the cursor. - (updated_at, id) < ( + (CASE WHEN pin_order > 0 THEN 1 ELSE 0 END, -pin_order, updated_at, id) < ( SELECT - updated_at, id + CASE WHEN c2.pin_order > 0 THEN 1 ELSE 0 END, -c2.pin_order, c2.updated_at, c2.id FROM - chats + chats c2 WHERE - id = $3 + c2.id = $3 ) ) ELSE true @@ -5843,9 +5841,15 @@ WHERE -- Authorize Filter clause will be injected below in GetAuthorizedChats -- @authorize_filter ORDER BY - -- Deterministic and consistent ordering of all rows, even if they share - -- a timestamp. This is to ensure consistent pagination. - (updated_at, id) DESC OFFSET $5 + -- Pinned chats (pin_order > 0) sort before unpinned ones. Within + -- pinned chats, lower pin_order values come first. The negation + -- trick (-pin_order) keeps all sort columns DESC so the cursor + -- tuple < comparison works with uniform direction. + CASE WHEN pin_order > 0 THEN 1 ELSE 0 END DESC, + -pin_order DESC, + updated_at DESC, + id DESC +OFFSET $5 LIMIT -- The chat list is unbounded and expected to grow large. -- Default to 50 to prevent accidental excessively large queries. diff --git a/coderd/database/queries/chats.sql b/coderd/database/queries/chats.sql index c54a8d51fa..a47d4bcc9a 100644 --- a/coderd/database/queries/chats.sql +++ b/coderd/database/queries/chats.sql @@ -353,20 +353,18 @@ WHERE ELSE chats.archived = sqlc.narg('archived') :: boolean END AND CASE - -- This allows using the last element on a page as effectively a cursor. - -- This is an important option for scripts that need to paginate without - -- duplicating or missing data. + -- Cursor pagination: the last element on a page acts as the cursor. + -- The 4-tuple matches the ORDER BY below. All columns sort DESC + -- (pin_order is negated so lower values sort first in DESC order), + -- which lets us use a single tuple < comparison. WHEN @after_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN ( - -- The pagination cursor is the last ID of the previous page. - -- The query is ordered by the updated_at field, so select all - -- rows before the cursor. - (updated_at, id) < ( + (CASE WHEN pin_order > 0 THEN 1 ELSE 0 END, -pin_order, updated_at, id) < ( SELECT - updated_at, id + CASE WHEN c2.pin_order > 0 THEN 1 ELSE 0 END, -c2.pin_order, c2.updated_at, c2.id FROM - chats + chats c2 WHERE - id = @after_id + c2.id = @after_id ) ) ELSE true @@ -378,9 +376,15 @@ WHERE -- Authorize Filter clause will be injected below in GetAuthorizedChats -- @authorize_filter ORDER BY - -- Deterministic and consistent ordering of all rows, even if they share - -- a timestamp. This is to ensure consistent pagination. - (updated_at, id) DESC OFFSET @offset_opt + -- Pinned chats (pin_order > 0) sort before unpinned ones. Within + -- pinned chats, lower pin_order values come first. The negation + -- trick (-pin_order) keeps all sort columns DESC so the cursor + -- tuple < comparison works with uniform direction. + CASE WHEN pin_order > 0 THEN 1 ELSE 0 END DESC, + -pin_order DESC, + updated_at DESC, + id DESC +OFFSET @offset_opt LIMIT -- The chat list is unbounded and expected to grow large. -- Default to 50 to prevent accidental excessively large queries. diff --git a/coderd/exp_chats.go b/coderd/exp_chats.go index deb554fcc8..dee223cab3 100644 --- a/coderd/exp_chats.go +++ b/coderd/exp_chats.go @@ -1810,9 +1810,9 @@ func (api *API) patchChat(rw http.ResponseWriter, r *http.Request) { // - pinOrder > 0 && already pinned: reorder (shift // neighbors, clamp to [1, count]). // - pinOrder > 0 && not pinned: append to end. The - // requested value is intentionally ignored because - // PinChatByID also bumps updated_at to keep the - // chat visible in the paginated sidebar. + // requested value is intentionally ignored; the + // SQL ORDER BY sorts pinned chats first so they + // appear on page 1 of the paginated sidebar. var err error errMsg := "Failed to pin chat." switch { diff --git a/coderd/exp_chats_test.go b/coderd/exp_chats_test.go index 2f5170957e..e5ea8b7e16 100644 --- a/coderd/exp_chats_test.go +++ b/coderd/exp_chats_test.go @@ -876,6 +876,186 @@ func TestListChats(t *testing.T) { require.NoError(t, err) require.Len(t, allChats, totalChats) }) + + // Test that a pinned chat with an old updated_at appears on page 1. + t.Run("PinnedOnFirstPage", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + client, _ := newChatClientWithDatabase(t) + _ = coderdtest.CreateFirstUser(t, client.Client) + _ = createChatModelConfig(t, client) + + // Create the chat that will later be pinned. It gets the + // earliest updated_at because it is inserted first. + pinnedChat, err := client.CreateChat(ctx, codersdk.CreateChatRequest{ + Content: []codersdk.ChatInputPart{{ + Type: codersdk.ChatInputPartTypeText, + Text: "pinned-chat", + }}, + }) + require.NoError(t, err) + + // Fill page 1 with newer chats so the pinned chat would + // normally be pushed off the first page (default limit 50). + const fillerCount = 51 + fillerChats := make([]codersdk.Chat, 0, fillerCount) + for i := range fillerCount { + c, createErr := client.CreateChat(ctx, codersdk.CreateChatRequest{ + Content: []codersdk.ChatInputPart{{ + Type: codersdk.ChatInputPartTypeText, + Text: fmt.Sprintf("filler-%d", i), + }}, + }) + require.NoError(t, createErr) + fillerChats = append(fillerChats, c) + } + + // Wait for all chats to reach a terminal status so + // updated_at is stable before paginating. A single + // polling loop checks every chat per tick to avoid + // O(N) separate Eventually loops. + allCreated := append([]codersdk.Chat{pinnedChat}, fillerChats...) + pending := make(map[uuid.UUID]struct{}, len(allCreated)) + for _, c := range allCreated { + pending[c.ID] = struct{}{} + } + testutil.Eventually(ctx, t, func(_ context.Context) bool { + all, listErr := client.ListChats(ctx, &codersdk.ListChatsOptions{ + Pagination: codersdk.Pagination{Limit: fillerCount + 10}, + }) + if listErr != nil { + return false + } + for _, ch := range all { + if _, ok := pending[ch.ID]; ok && ch.Status != codersdk.ChatStatusPending && ch.Status != codersdk.ChatStatusRunning { + delete(pending, ch.ID) + } + } + return len(pending) == 0 + }, testutil.IntervalFast) + + // Pin the earliest chat. + err = client.UpdateChat(ctx, pinnedChat.ID, codersdk.UpdateChatRequest{ + PinOrder: ptr.Ref(int32(1)), + }) + require.NoError(t, err) + + // Fetch page 1 with default limit (50). + page1, err := client.ListChats(ctx, &codersdk.ListChatsOptions{ + Pagination: codersdk.Pagination{Limit: 50}, + }) + require.NoError(t, err) + + // The pinned chat must appear on page 1. + page1IDs := make(map[uuid.UUID]struct{}, len(page1)) + for _, c := range page1 { + page1IDs[c.ID] = struct{}{} + } + _, found := page1IDs[pinnedChat.ID] + require.True(t, found, "pinned chat should appear on page 1") + + // The pinned chat should be the first item in the list. + require.Equal(t, pinnedChat.ID, page1[0].ID, "pinned chat should be first") + }) + + // Test cursor pagination with a mix of pinned and unpinned chats. + t.Run("CursorWithPins", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + client, _ := newChatClientWithDatabase(t) + _ = coderdtest.CreateFirstUser(t, client.Client) + _ = createChatModelConfig(t, client) + + // Create 5 chats: 2 will be pinned, 3 unpinned. + const totalChats = 5 + createdChats := make([]codersdk.Chat, 0, totalChats) + for i := range totalChats { + c, createErr := client.CreateChat(ctx, codersdk.CreateChatRequest{ + Content: []codersdk.ChatInputPart{{ + Type: codersdk.ChatInputPartTypeText, + Text: fmt.Sprintf("cursor-pin-chat-%d", i), + }}, + }) + require.NoError(t, createErr) + createdChats = append(createdChats, c) + } + + // Wait for all chats to reach terminal status. + // Check each chat by ID rather than fetching the full list. + testutil.Eventually(ctx, t, func(_ context.Context) bool { + for _, c := range createdChats { + ch, err := client.GetChat(ctx, c.ID) + require.NoError(t, err, "GetChat should succeed for just-created chat %s", c.ID) + if ch.Status == codersdk.ChatStatusPending || ch.Status == codersdk.ChatStatusRunning { + return false + } + } + return true + }, testutil.IntervalFast) + + // Pin the first two chats (oldest updated_at). + err := client.UpdateChat(ctx, createdChats[0].ID, codersdk.UpdateChatRequest{ + PinOrder: ptr.Ref(int32(1)), + }) + require.NoError(t, err) + err = client.UpdateChat(ctx, createdChats[1].ID, codersdk.UpdateChatRequest{ + PinOrder: ptr.Ref(int32(1)), + }) + require.NoError(t, err) + + // Paginate with limit=2 using cursor (after_id). + const pageSize = 2 + maxPages := totalChats/pageSize + 2 + var allPaginated []codersdk.Chat + var afterID uuid.UUID + for range maxPages { + opts := &codersdk.ListChatsOptions{ + Pagination: codersdk.Pagination{Limit: pageSize}, + } + if afterID != uuid.Nil { + opts.Pagination.AfterID = afterID + } + page, listErr := client.ListChats(ctx, opts) + require.NoError(t, listErr) + if len(page) == 0 { + break + } + allPaginated = append(allPaginated, page...) + afterID = page[len(page)-1].ID + } + + // All chats should appear exactly once. + seenIDs := make(map[uuid.UUID]struct{}, len(allPaginated)) + for _, c := range allPaginated { + _, dup := seenIDs[c.ID] + require.False(t, dup, "chat %s appeared more than once", c.ID) + seenIDs[c.ID] = struct{}{} + } + require.Len(t, seenIDs, totalChats, "all chats should appear in paginated results") + + // Pinned chats should come before unpinned ones, and + // within the pinned group, lower pin_order sorts first. + pinnedSeen := false + unpinnedSeen := false + for _, c := range allPaginated { + if c.PinOrder > 0 { + require.False(t, unpinnedSeen, "pinned chat %s appeared after unpinned chat", c.ID) + pinnedSeen = true + } else { + unpinnedSeen = true + } + } + require.True(t, pinnedSeen, "at least one pinned chat should exist") + + // Verify within-pinned ordering: pin_order=1 before + // pin_order=2 (the -pin_order DESC column). + require.Equal(t, createdChats[0].ID, allPaginated[0].ID, + "pin_order=1 chat should be first") + require.Equal(t, createdChats[1].ID, allPaginated[1].ID, + "pin_order=2 chat should be second") + }) } func TestListChatModels(t *testing.T) {