From 5040ab6fcab03763c010e81b868f619128cdacf7 Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Wed, 13 May 2026 11:06:42 -0400 Subject: [PATCH] feat: filter chats by diff URL via the q search parameter (#24970) Adds a `diff_url:` term to the `q` search parameter on `GET /api/experimental/chats` so callers can look up the chat associated with a particular pull request, merge request, or any other URL persisted on the chat's diff status. ``` q=diff_url:"https://github.com/coder/coder/pull/123" ``` Match is case-insensitive. When the URL lives on a delegated sub-agent's diff status, the parent chat is returned so the relationship surfaces from a single lookup.
Design notes - **Forge-agnostic.** Reuses the existing `chat_diff_statuses.url` column rather than introducing a `pr:` vocabulary, since the SDK already documents the URL as "may point to a pull request or a branch page depending on whether a PR has been opened." Works for GitHub PRs, GitLab MRs, branch pages, etc. - **Composes with `archived:`.** The two terms can be combined: `q=archived:true diff_url:"..."`. - **Case handling.** The parser used to lowercase the entire `q` string up front, which would mangle URL path segments. Switched to lowercasing only the field key inside `searchTerms` (already happens there) and keeping the value as the caller typed it. The SQL comparison lowercases on both sides. - **Validation.** `diff_url` must be a syntactically valid HTTP(S) URL with a non-empty host. No forge-specific validation. - **Index.** Adds `idx_chat_diff_statuses_url_lower` on `LOWER(url)` so the lookup is cheap even on large datasets. - **Sub-agent fan-in.** `EXISTS` clause matches when the URL lives on the chat itself or any chat with `root_chat_id` equal to the chat's id, so a delegated sub-agent's PR pulls in its parent. - **Deferred.** Sentinels like `pr:any` / `pr:none` and a forge-agnostic state filter (`diff_state:open|merged|closed`) were intentionally left out of this change. They couple cleanly to a second forge or a clearer product call, and shipping them now would lock in vocabulary we may want to revisit.
## Tests - `coderd/searchquery`: parser tests for valid URLs, case handling (key insensitive, value preserved), composition with `archived:`, and validation errors (non-HTTP scheme, missing host, malformed URL). - `coderd/exp_chats_test.go`: end-to-end coverage hitting `ListChats`. Verifies a root chat matches its own URL, a parent chat surfaces when only a sub-agent has the URL, lookups are case-insensitive, non-matching URLs return empty, and invalid URLs return `400`. --- _This PR was authored by a Coder Agent on behalf of @kylecarbs._ --- coderd/apidoc/docs.go | 2 +- coderd/apidoc/swagger.json | 2 +- coderd/database/dump.sql | 2 + ..._idx_chat_diff_statuses_url_lower.down.sql | 1 + ...93_idx_chat_diff_statuses_url_lower.up.sql | 5 + coderd/database/modelqueries.go | 1 + coderd/database/queries.sql.go | 22 ++- coderd/database/queries/chats.sql | 16 +++ coderd/database/sqlc.yaml | 1 + coderd/exp_chats.go | 3 +- coderd/exp_chats_test.go | 130 ++++++++++++++++++ coderd/searchquery/search.go | 36 ++++- coderd/searchquery/search_test.go | 70 ++++++++++ docs/reference/api/chats.md | 8 +- 14 files changed, 288 insertions(+), 11 deletions(-) create mode 100644 coderd/database/migrations/000493_idx_chat_diff_statuses_url_lower.down.sql create mode 100644 coderd/database/migrations/000493_idx_chat_diff_statuses_url_lower.up.sql diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 679fb97605..b4b2a60160 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -78,7 +78,7 @@ const docTemplate = `{ "parameters": [ { "type": "string", - "description": "Search query", + "description": "Search query. Supports archived:bool and diff_url:\u003curl\u003e terms (quote URLs).", "name": "q", "in": "query" }, diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index be636fd5c8..a77910edd7 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -59,7 +59,7 @@ "parameters": [ { "type": "string", - "description": "Search query", + "description": "Search query. Supports archived:bool and diff_url:\u003curl\u003e terms (quote URLs).", "name": "q", "in": "query" }, diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index ef76f01353..3c60581d5e 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -3851,6 +3851,8 @@ CREATE INDEX idx_chat_debug_steps_stale ON chat_debug_steps USING btree (updated CREATE INDEX idx_chat_diff_statuses_stale_at ON chat_diff_statuses USING btree (stale_at); +CREATE INDEX idx_chat_diff_statuses_url_lower ON chat_diff_statuses USING btree (lower(url)) WHERE ((url IS NOT NULL) AND (url <> ''::text)); + CREATE INDEX idx_chat_file_links_chat_id ON chat_file_links USING btree (chat_id); CREATE INDEX idx_chat_files_org ON chat_files USING btree (organization_id); diff --git a/coderd/database/migrations/000493_idx_chat_diff_statuses_url_lower.down.sql b/coderd/database/migrations/000493_idx_chat_diff_statuses_url_lower.down.sql new file mode 100644 index 0000000000..1bda083b76 --- /dev/null +++ b/coderd/database/migrations/000493_idx_chat_diff_statuses_url_lower.down.sql @@ -0,0 +1 @@ +DROP INDEX IF EXISTS idx_chat_diff_statuses_url_lower; diff --git a/coderd/database/migrations/000493_idx_chat_diff_statuses_url_lower.up.sql b/coderd/database/migrations/000493_idx_chat_diff_statuses_url_lower.up.sql new file mode 100644 index 0000000000..4ab1eb17f7 --- /dev/null +++ b/coderd/database/migrations/000493_idx_chat_diff_statuses_url_lower.up.sql @@ -0,0 +1,5 @@ +-- Index on LOWER(url) supports case-insensitive lookups when filtering +-- chats by their associated diff URL (e.g. a pull request URL). +CREATE INDEX idx_chat_diff_statuses_url_lower + ON chat_diff_statuses (LOWER(url)) + WHERE url IS NOT NULL AND url <> ''; diff --git a/coderd/database/modelqueries.go b/coderd/database/modelqueries.go index 78331e338b..dcacc7ac9b 100644 --- a/coderd/database/modelqueries.go +++ b/coderd/database/modelqueries.go @@ -764,6 +764,7 @@ func (q *sqlQuerier) GetAuthorizedChats(ctx context.Context, arg GetChatsParams, arg.Archived, arg.AfterID, arg.LabelFilter, + arg.DiffURL, arg.OffsetOpt, arg.LimitOpt, ) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 1a3dae3838..eb5c6fc924 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -6992,6 +6992,22 @@ WHERE WHEN $4::jsonb IS NOT NULL THEN chats.labels @> $4::jsonb ELSE true END + -- Match chats whose linked diff URL (e.g. a pull request URL) + -- equals the given value, case-insensitively. The URL may live on + -- a delegated sub-agent's diff status, so we surface the root chat + -- when any descendant matches. + AND CASE + WHEN $5::text IS NOT NULL THEN EXISTS ( + SELECT 1 + FROM chat_diff_statuses cds + JOIN chats c2 ON c2.id = cds.chat_id + WHERE cds.url IS NOT NULL + AND cds.url <> '' + AND LOWER(cds.url) = LOWER($5::text) + AND (c2.id = chats.id OR c2.root_chat_id = chats.id) + ) + ELSE true + END -- Paginate over root chats only. Children are fetched -- separately via GetChildChatsByParentIDs and embedded under -- each parent. Other callers that need the full set should @@ -7008,11 +7024,11 @@ ORDER BY -pin_order DESC, updated_at DESC, id DESC -OFFSET $5 +OFFSET $6 LIMIT -- The chat list is unbounded and expected to grow large. -- Default to 50 to prevent accidental excessively large queries. - COALESCE(NULLIF($6 :: int, 0), 50) + COALESCE(NULLIF($7 :: int, 0), 50) ` type GetChatsParams struct { @@ -7020,6 +7036,7 @@ type GetChatsParams struct { Archived sql.NullBool `db:"archived" json:"archived"` AfterID uuid.UUID `db:"after_id" json:"after_id"` LabelFilter pqtype.NullRawMessage `db:"label_filter" json:"label_filter"` + DiffURL sql.NullString `db:"diff_url" json:"diff_url"` OffsetOpt int32 `db:"offset_opt" json:"offset_opt"` LimitOpt int32 `db:"limit_opt" json:"limit_opt"` } @@ -7035,6 +7052,7 @@ func (q *sqlQuerier) GetChats(ctx context.Context, arg GetChatsParams) ([]GetCha arg.Archived, arg.AfterID, arg.LabelFilter, + arg.DiffURL, arg.OffsetOpt, arg.LimitOpt, ) diff --git a/coderd/database/queries/chats.sql b/coderd/database/queries/chats.sql index 54aea614f9..23dec2dbf7 100644 --- a/coderd/database/queries/chats.sql +++ b/coderd/database/queries/chats.sql @@ -377,6 +377,22 @@ WHERE WHEN sqlc.narg('label_filter')::jsonb IS NOT NULL THEN chats.labels @> sqlc.narg('label_filter')::jsonb ELSE true END + -- Match chats whose linked diff URL (e.g. a pull request URL) + -- equals the given value, case-insensitively. The URL may live on + -- a delegated sub-agent's diff status, so we surface the root chat + -- when any descendant matches. + AND CASE + WHEN sqlc.narg('diff_url')::text IS NOT NULL THEN EXISTS ( + SELECT 1 + FROM chat_diff_statuses cds + JOIN chats c2 ON c2.id = cds.chat_id + WHERE cds.url IS NOT NULL + AND cds.url <> '' + AND LOWER(cds.url) = LOWER(sqlc.narg('diff_url')::text) + AND (c2.id = chats.id OR c2.root_chat_id = chats.id) + ) + ELSE true + END -- Paginate over root chats only. Children are fetched -- separately via GetChildChatsByParentIDs and embedded under -- each parent. Other callers that need the full set should diff --git a/coderd/database/sqlc.yaml b/coderd/database/sqlc.yaml index 33c017b535..0be8f26bbd 100644 --- a/coderd/database/sqlc.yaml +++ b/coderd/database/sqlc.yaml @@ -182,6 +182,7 @@ sql: api_version: APIVersion avatar_url: AvatarURL created_by_avatar_url: CreatedByAvatarURL + diff_url: DiffURL dbcrypt_key: DBCryptKey session_count_vscode: SessionCountVSCode session_count_jetbrains: SessionCountJetBrains diff --git a/coderd/exp_chats.go b/coderd/exp_chats.go index ffa2c3f8dc..d9f99c7df0 100644 --- a/coderd/exp_chats.go +++ b/coderd/exp_chats.go @@ -312,7 +312,7 @@ func (api *API) chatsByWorkspace(rw http.ResponseWriter, r *http.Request) { // @Security CoderSessionToken // @Tags Chats // @Produce json -// @Param q query string false "Search query" +// @Param q query string false "Search query. Supports archived:bool and diff_url: terms (quote URLs)." // @Param label query string false "Filter by label as key:value. Repeat for multiple (AND logic)." // @Success 200 {array} codersdk.Chat // @Router /api/experimental/chats [get] @@ -368,6 +368,7 @@ func (api *API) listChats(rw http.ResponseWriter, r *http.Request) { Archived: searchParams.Archived, AfterID: paginationParams.AfterID, LabelFilter: labelFilter, + DiffURL: searchParams.DiffURL, // #nosec G115 - Pagination offsets are small and fit in int32 OffsetOpt: int32(paginationParams.Offset), // #nosec G115 - Pagination limits are small and fit in int32 diff --git a/coderd/exp_chats_test.go b/coderd/exp_chats_test.go index 3686b9795a..5c860008ac 100644 --- a/coderd/exp_chats_test.go +++ b/coderd/exp_chats_test.go @@ -1409,6 +1409,136 @@ func TestListChats(t *testing.T) { require.Len(t, c.Children, 2, "each root should embed its 2 children") } }) + + t.Run("DiffURLFilter", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + client, db := newChatClientWithDatabase(t) + user := coderdtest.CreateFirstUser(t, client.Client) + modelConfig := createChatModelConfig(t, client) + + // Helper that creates a chat (root or child) with a diff status URL. + create := func(title, url string, parentID uuid.NullUUID) database.Chat { + rootID := parentID + chat := dbgen.Chat(t, db, database.Chat{ + OrganizationID: user.OrganizationID, + OwnerID: user.UserID, + LastModelConfigID: modelConfig.ID, + Title: title, + ParentChatID: parentID, + RootChatID: rootID, + }) + if url != "" { + staleAt := time.Now().UTC().Add(time.Hour).Truncate(time.Second) + _, err := db.UpsertChatDiffStatusReference( + dbauthz.AsSystemRestricted(ctx), + database.UpsertChatDiffStatusReferenceParams{ + ChatID: chat.ID, + Url: sql.NullString{String: url, Valid: true}, + GitBranch: "feature/test", + GitRemoteOrigin: "git@github.com:coder/coder.git", + StaleAt: staleAt, + }, + ) + require.NoError(t, err) + } + return chat + } + + // Root chat directly linked to the target PR. + rootWithPR := create("root with pr", "https://github.com/coder/coder/pull/1", uuid.NullUUID{}) + + // Root chat whose sub-agent owns the PR. The filter should still + // surface the parent because the URL lives on a descendant. + rootWithChildPR := create("root with child pr", "", uuid.NullUUID{}) + _ = create( + "sub-agent with pr", + "https://github.com/coder/coder/pull/2", + uuid.NullUUID{UUID: rootWithChildPR.ID, Valid: true}, + ) + + // Root chat with an unrelated PR; should not match either filter. + _ = create("unrelated pr", "https://github.com/coder/coder/pull/999", uuid.NullUUID{}) + + // Root chat with no diff status at all. + _ = create("no diff", "", uuid.NullUUID{}) + + // Archived root chat that points at the same URL as `rootWithPR`. + // Used to verify the archived filter and the diff_url filter + // compose at the SQL layer rather than ignoring each other. + archivedWithPR := create( + "archived with pr", + "https://github.com/coder/coder/pull/3", + uuid.NullUUID{}, + ) + require.NoError(t, client.UpdateChat(ctx, archivedWithPR.ID, codersdk.UpdateChatRequest{ + Archived: ptr.Ref(true), + })) + + t.Run("MatchesRoot", func(t *testing.T) { + chats, err := client.ListChats(ctx, &codersdk.ListChatsOptions{ + Query: `diff_url:"https://github.com/coder/coder/pull/1"`, + }) + require.NoError(t, err) + require.Len(t, chats, 1) + require.Equal(t, rootWithPR.ID, chats[0].ID) + }) + + t.Run("MatchesViaSubAgent", func(t *testing.T) { + chats, err := client.ListChats(ctx, &codersdk.ListChatsOptions{ + Query: `diff_url:"https://github.com/coder/coder/pull/2"`, + }) + require.NoError(t, err) + require.Len(t, chats, 1, "root chat should surface even when only a child has the PR") + require.Equal(t, rootWithChildPR.ID, chats[0].ID) + }) + + t.Run("CaseInsensitive", func(t *testing.T) { + chats, err := client.ListChats(ctx, &codersdk.ListChatsOptions{ + Query: `diff_url:"HTTPS://GITHUB.COM/CODER/CODER/PULL/1"`, + }) + require.NoError(t, err) + require.Len(t, chats, 1) + require.Equal(t, rootWithPR.ID, chats[0].ID) + }) + + t.Run("NoMatch", func(t *testing.T) { + chats, err := client.ListChats(ctx, &codersdk.ListChatsOptions{ + Query: `diff_url:"https://github.com/coder/coder/pull/424242"`, + }) + require.NoError(t, err) + require.Empty(t, chats) + }) + + t.Run("InvalidURL", func(t *testing.T) { + _, err := client.ListChats(ctx, &codersdk.ListChatsOptions{ + Query: `diff_url:"ftp://example.com/x"`, + }) + sdkErr := requireSDKError(t, err, http.StatusBadRequest) + require.NotEmpty(t, sdkErr.Validations, "expected validation error") + require.Equal(t, "diff_url", sdkErr.Validations[0].Field) + }) + + t.Run("ArchivedFilteredOut", func(t *testing.T) { + // Default archived filter is false, so an archived chat with + // a matching diff URL must not surface. + chats, err := client.ListChats(ctx, &codersdk.ListChatsOptions{ + Query: `diff_url:"https://github.com/coder/coder/pull/3"`, + }) + require.NoError(t, err) + require.Empty(t, chats, "archived chat must not match the default filter") + }) + + t.Run("ArchivedTrueComposes", func(t *testing.T) { + chats, err := client.ListChats(ctx, &codersdk.ListChatsOptions{ + Query: `archived:true diff_url:"https://github.com/coder/coder/pull/3"`, + }) + require.NoError(t, err) + require.Len(t, chats, 1) + require.Equal(t, archivedWithPR.ID, chats[0].ID) + }) + }) } func TestListChatModels(t *testing.T) { diff --git a/coderd/searchquery/search.go b/coderd/searchquery/search.go index 260ba792fc..f594856d8c 100644 --- a/coderd/searchquery/search.go +++ b/coderd/searchquery/search.go @@ -544,6 +544,9 @@ func Tasks(ctx context.Context, db database.Store, query string, actorID uuid.UU // // Supported query parameters: // - archived: boolean (default: false, excludes archived chats unless explicitly set) +// - diff_url: string (matches chats whose linked diff URL equals the +// given value, case-insensitively; URLs typically contain ':' so +// they must be quoted, e.g. q=diff_url:"https://github.com/o/r/pull/1") func Chats(query string) (database.GetChatsParams, []codersdk.ValidationError) { filter := database.GetChatsParams{ // Default to hiding archived chats. @@ -554,8 +557,10 @@ func Chats(query string) (database.GetChatsParams, []codersdk.ValidationError) { return filter, nil } - // Always lowercase for all searches. - query = strings.ToLower(query) + // Lowercase the keys so they match regardless of how the caller + // types them, but preserve value casing because some filters + // (e.g. diff_url) may include URL path segments where case is + // meaningful. values, errors := searchTerms(query, func(term string, _ url.Values) error { return xerrors.Errorf("unsupported search term: %q", term) }) @@ -565,11 +570,38 @@ func Chats(query string) (database.GetChatsParams, []codersdk.ValidationError) { parser := httpapi.NewQueryParamParser() filter.Archived = parser.NullableBoolean(values, filter.Archived, "archived") + if diffURL := parser.String(values, "", "diff_url"); diffURL != "" { + if err := validateDiffURL(diffURL); err != nil { + parser.Errors = append(parser.Errors, codersdk.ValidationError{ + Field: "diff_url", + Detail: err.Error(), + }) + } else { + filter.DiffURL = sql.NullString{String: diffURL, Valid: true} + } + } parser.ErrorExcessParams(values) return filter, parser.Errors } +// validateDiffURL checks that the value is a syntactically valid HTTP(S) +// URL. The check is intentionally forge-agnostic because the diff URL on +// a chat may point to a pull request, merge request, branch page, etc. +func validateDiffURL(raw string) error { + u, err := url.Parse(raw) + if err != nil { + return xerrors.Errorf("diff_url is not a valid URL: %w", err) + } + if u.Scheme != "http" && u.Scheme != "https" { + return xerrors.Errorf("diff_url must use http or https scheme, got %q", u.Scheme) + } + if u.Host == "" { + return xerrors.New("diff_url must include a host") + } + return nil +} + func searchTerms(query string, defaultKey func(term string, values url.Values) error) (url.Values, []codersdk.ValidationError) { searchValues := make(url.Values) diff --git a/coderd/searchquery/search_test.go b/coderd/searchquery/search_test.go index 8e6013ad5a..05b801c158 100644 --- a/coderd/searchquery/search_test.go +++ b/coderd/searchquery/search_test.go @@ -1239,6 +1239,17 @@ func TestSearchChats(t *testing.T) { Archived: sql.NullBool{Bool: true, Valid: true}, }, }, + { + // Documents that uppercase boolean values still parse. The Chats + // parser intentionally does not pre-lowercase the query because + // diff_url path segments are case-meaningful, so this guards + // against regressions if the blanket lowercase is ever re-added. + Name: "ArchivedTrueUpperCase", + Query: "archived:TRUE", + Expected: database.GetChatsParams{ + Archived: sql.NullBool{Bool: true, Valid: true}, + }, + }, { Name: "ArchivedFalse", Query: "archived:false", @@ -1266,6 +1277,65 @@ func TestSearchChats(t *testing.T) { Query: "archived:", ExpectedErrorContains: "cannot start or end with ':'", }, + { + Name: "DiffURL", + Query: `diff_url:"https://github.com/coder/coder/pull/123"`, + Expected: database.GetChatsParams{ + Archived: sql.NullBool{Bool: false, Valid: true}, + DiffURL: sql.NullString{ + String: "https://github.com/coder/coder/pull/123", + Valid: true, + }, + }, + }, + { + Name: "DiffURLPreservesValueCase", + Query: `diff_url:"https://github.com/Coder/Coder/pull/123"`, + Expected: database.GetChatsParams{ + Archived: sql.NullBool{Bool: false, Valid: true}, + DiffURL: sql.NullString{ + String: "https://github.com/Coder/Coder/pull/123", + Valid: true, + }, + }, + }, + { + Name: "DiffURLKeyCaseInsensitive", + Query: `Diff_URL:"https://github.com/coder/coder/pull/1"`, + Expected: database.GetChatsParams{ + Archived: sql.NullBool{Bool: false, Valid: true}, + DiffURL: sql.NullString{ + String: "https://github.com/coder/coder/pull/1", + Valid: true, + }, + }, + }, + { + Name: "DiffURLWithArchived", + Query: `archived:true diff_url:"https://gitlab.com/foo/bar/-/merge_requests/9"`, + Expected: database.GetChatsParams{ + Archived: sql.NullBool{Bool: true, Valid: true}, + DiffURL: sql.NullString{ + String: "https://gitlab.com/foo/bar/-/merge_requests/9", + Valid: true, + }, + }, + }, + { + Name: "DiffURLInvalidScheme", + Query: `diff_url:"ftp://example.com/x"`, + ExpectedErrorContains: "http or https scheme", + }, + { + Name: "DiffURLMissingHost", + Query: `diff_url:"https:///pull/1"`, + ExpectedErrorContains: "must include a host", + }, + { + Name: "DiffURLMalformed", + Query: `diff_url:"http://%41:8080/"`, + ExpectedErrorContains: "not a valid URL", + }, } for _, c := range testCases { diff --git a/docs/reference/api/chats.md b/docs/reference/api/chats.md index 15ecdd595d..90f7460d90 100644 --- a/docs/reference/api/chats.md +++ b/docs/reference/api/chats.md @@ -17,10 +17,10 @@ Experimental: this endpoint is subject to change. ### Parameters -| Name | In | Type | Required | Description | -|---------|-------|--------|----------|----------------------------------------------------------------| -| `q` | query | string | false | Search query | -| `label` | query | string | false | Filter by label as key:value. Repeat for multiple (AND logic). | +| Name | In | Type | Required | Description | +|---------|-------|--------|----------|-----------------------------------------------------------------------------| +| `q` | query | string | false | Search query. Supports archived:bool and diff_url: terms (quote URLs). | +| `label` | query | string | false | Filter by label as key:value. Repeat for multiple (AND logic). | ### Example responses