Repository navigation
feat: wire chat search box to full-text search #27973
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
7fc93ab
2ee34e5
86445a0
026020b
3132d51
1fcef68
5a925b1
f0bd801
4a47b35
b1846b7
450d239
dbfde13
60a3a6d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
…arch The Coder Agents chat search dialog sent bare free text as a title substring filter (title:"..."). Point it at the backend full-text search filter (search:) so free text matches chat titles, PR titles, PR numbers, and message bodies. Bare free text is wrapped in a quoted phrase by default, since the backend query tokenizer requires the search value to be a single token. Websearch operators (quoted phrases, OR, -negation) still pass through when the user supplies a proper quoted phrase. The empty state notes that message content is indexed periodically.
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,8 @@ | ||
| // The backend's search-query parser toggles its quoted-state on every `"` and | ||
| // has no backslash-escape handling, so escaping quotes here would produce a | ||
| // query the backend cannot parse. Stripping quotes from bare text keeps the | ||
| // resulting `title:"..."` filter well-formed for the backend. | ||
| // query the backend cannot parse. Stripping quotes from structured filter | ||
| // values keeps the resulting `key:"..."` token well-formed for the backend. | ||
| // Bare free text is not sanitized this way so that FTS quoted phrases survive. | ||
| const sanitizeChatSearchValue = (value: string): string => { | ||
| return value.replaceAll('"', ""); | ||
| }; | ||
|
|
@@ -10,9 +11,32 @@ const addDefaultURLScheme = (value: string): string => { | |
| return /^[a-z][a-z\d+\-.]*:\/\//i.test(value) ? value : `https://${value}`; | ||
| }; | ||
|
|
||
| // Bare free text may contain websearch operators (quoted phrases, OR, | ||
| // -negation). Detect a leading/trailing quote pair so those pass through | ||
| // unmodified; everything else gets wrapped in a single quoted phrase. | ||
| const hasWebSearchQuotes = (value: string): boolean => { | ||
| const first = value.indexOf('"'); | ||
| const last = value.lastIndexOf('"'); | ||
| return ( | ||
| first !== -1 && last > first && /\S/.test(value.slice(first + 1, last)) | ||
| ); | ||
| }; | ||
|
|
||
| // Wrap bare free text in a quoted phrase so multi-word input reaches the | ||
| // backend's FTS filter as a single token. Quotes are stripped first because | ||
| // the backend's query parser has no escape handling for embedded quotes. | ||
| const toSearchPhrase = (terms: string): string => { | ||
| const joined = terms.trim(); | ||
| if (hasWebSearchQuotes(joined)) { | ||
| return joined; | ||
| } | ||
| return `"${sanitizeChatSearchValue(joined)}"`; | ||
| }; | ||
|
|
||
| // Filter keys that may pass through to the backend unchanged. `title` is not | ||
| // listed here because bare text and `title:` filters are merged into a single | ||
| // title filter; see the title-handling branch in normalizeChatSearchInput. | ||
| // FTS `search:` filter; see the search-handling branch in | ||
| // normalizeChatSearchInput. | ||
| const passthroughChatSearchFilterKeys = new Set([ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note [CRF-18] Backend-supported filters
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Acknowledged, out of scope.
|
||
| "archived", | ||
| "diff_url", | ||
|
|
@@ -107,7 +131,7 @@ const normalizePassthroughChatSearchFilter = ({ | |
| /** | ||
| * Normalizes raw search input into a query string the chat search API accepts. | ||
| * | ||
| * Bare text and `title:` filters are merged into a single `title:"..."` | ||
| * Bare text and `title:` filters are merged into a single `search:` FTS | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2 [CRF-3] The doc comment says The exported contract reads "Bare text and Leorio's prescription: "Bare text becomes a single
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved by removal. The
|
||
| * filter (the backend rejects a parameter that appears more than once). | ||
| * Recognized `key:value` filters are normalized for backend syntax. | ||
| */ | ||
|
|
@@ -122,26 +146,26 @@ export const normalizeChatSearchInput = ( | |
| const tokens = splitSearchInput(trimmedInput); | ||
| const passthroughFilters: string[] = []; | ||
| const normalizedTokens: string[] = []; | ||
| const titleTerms: string[] = []; | ||
| let hasBareTitleText = false; | ||
| const searchTerms: string[] = []; | ||
| let hasBareSearchText = false; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit [CRF-11] Lines 160-162 set it when
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved by the refactor.
|
||
|
|
||
| for (const token of tokens) { | ||
| const keyValuePair = getKeyValuePair(token); | ||
| if (!keyValuePair) { | ||
| titleTerms.push(token); | ||
| hasBareTitleText = true; | ||
| searchTerms.push(token); | ||
| hasBareSearchText = true; | ||
| continue; | ||
| } | ||
|
|
||
| if (keyValuePair.key === "title") { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note [CRF-19] The class fix for the title-fold inconsistency lives one layer down, and it is a human decision. (Meruem) The same
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed this is a human decision and out of this PR's scope. We removed the
|
||
| normalizedTokens.push(token); | ||
| titleTerms.push(keyValuePair.value); | ||
| searchTerms.push(keyValuePair.value); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3 [CRF-9] An explicit
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved by removal.
|
||
| continue; | ||
| } | ||
|
|
||
| if (!passthroughChatSearchFilterKeys.has(keyValuePair.key)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3 [CRF-6] A user-typed
Fix: handle
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3 [CRF-29] This reply opens with "Fixed." while the flagged behavior is retained and defended. (Mafu-san) The first word claims a fix; the following sentences accurately describe the unchanged wire output ( For the record, the panel evaluated the underlying defense and accepted it 7/7: the literal-text rule is uniform across all non-pill keys, the typed text stays visible in the box, and special-casing
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This thread is the blocker for the next review round. The reply above opens with "Fixed." while the behavior it describes is retained (and the panel has since accepted that design decision, so no code change is being requested). What is needed: correct the label on the record, or state that it stands. A reply header of "Fixed" on unchanged behavior misleads anyone scanning this thread later.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correction to my earlier reply on this thread: it opened with "Fixed." but the behavior did not change. Typed
|
||
| titleTerms.push(token); | ||
| hasBareTitleText = true; | ||
| searchTerms.push(token); | ||
| hasBareSearchText = true; | ||
| continue; | ||
| } | ||
|
|
||
|
|
@@ -150,18 +174,20 @@ export const normalizeChatSearchInput = ( | |
| normalizedTokens.push(normalizedFilter); | ||
| } | ||
|
|
||
| // Multiple title values must be merged into a single title filter because | ||
| // Multiple search values must be merged into a single search filter because | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3 [CRF-10] The "backend rejects a repeated parameter" rationale is stated three times in this file, and the "parser has no escape handling for quotes" rationale three times across this file and the test. (Gon) Instances of the repeated-parameter rule: line 112 (doc comment), line 140 (title branch), lines 158-159 (merge guard). Instances of the quote-handling rule: lines 1-4, lines 171-172, and searchQuery.test.ts:82. Each why belongs in one owning place; the copies will drift when the backend parser changes, and stale copies then mislead (CRF-4 is the live example: one of the copies is already wrong). State each rationale once, the doc comment for the merge rule and the Related, same class, four more comments restate what the code or the assertions already show: searchQuery.ts:1-4 (second sentence restates the first), searchQuery.ts:168-172 (final clause duplicates the sanitize header), ChatSearchDialog.tsx:134-136 (narrates the body and restates the callee's contract), searchQuery.test.ts:82 and :105 (trailing clauses restate the expectations). Trimming each to its owning rationale would cut six of the eleven touched comments roughly in half.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed. The refactor deleted the duplicated rationales along with the two-pass parser. The surviving comments each own one fact: the quote-strip header on
|
||
| // the backend's query parser rejects the same key appearing more than once. | ||
| if (titleTerms.length > 1) { | ||
| hasBareTitleText = true; | ||
| if (searchTerms.length > 1) { | ||
| hasBareSearchText = true; | ||
| } | ||
|
|
||
| if (!hasBareTitleText) { | ||
| if (!hasBareSearchText) { | ||
| return normalizedTokens.join(" "); | ||
| } | ||
|
|
||
| // Free text defaults to the backend's full-text search filter, which | ||
| // matches chat titles, PR titles, and message bodies. | ||
| return [ | ||
| ...passthroughFilters, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3 [CRF-7] The merge branch returns Every token appended to Fix: eliminate the shadow list. Track
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved by the refactor. The shadow-list invariant is gone:
|
||
| `title:"${sanitizeChatSearchValue(titleTerms.join(" "))}"`, | ||
| `search:${toSearchPhrase(searchTerms.join(" "))}`, | ||
| ].join(" "); | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3 [CRF-8] The indexing-lag explanation is shown for every empty result set, including filter-only queries where FTS is never involved. (Mafuuu P3, Leorio P3, Meruem P3, Bisky Note, Hisoka Note, Pariston Note, Gon Note, Nami Note, Komugi Note)
hasQueryis true for filter-only queries (hasActiveSearch = effectiveFilters.length > 0 || freeText.trim() !== "", ChatSearchDialog.tsx:189). A user who clicks the "Unread" pill and gets zero results reads "Message content is indexed periodically, so very recent messages may not be searchable yet." Indexing lag has nothing to do withhas_unread:true; that filter reads live columns and never consultssearch_tsv. Same for a puretitle:"..."search (live ILIKE, zero lag) and for chat-title/PR-title matching undersearch:, which computeto_tsvectorinline (chats.sql:695-702); only message bodies lag. Telling that user to wait for indexing sends them chasing a cause that does not exist.Fix: pass a boolean (e.g.
hasSearchText) down from the dialog, which already knowsqueryFreeText, and append the indexing sentence only when free text contributed to the query.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
buildChatSearchQuerynow returnshasSearchText(true only when asearch:token was actually emitted), andChatSearchResultsshows the indexing note only whenhasSearchTextis true. Filter-only queries (e.g.has_unread:true) and punctuation-only text no longer show the note. Covered by thePunctuationOnlyTextHidesIndexingNotestory.