feat: modal chat search popup#25535
Conversation
Cherry-picked backend changes from b924883 (Danielle Maywood). Adds chat_status:unread and pr_status:<draft|open|merged|closed> filters.
…s in exp_chats_test
|
/coder-agents-review |
There was a problem hiding this comment.
First-pass review (Netero). 1 P2, 1 P3. Both are mechanical: a missing accessibility primitive and a duplicated utility.
This is a first-pass review only. These are mechanical findings from Netero; the full review panel has not yet reviewed this PR. The panel will review after these findings are addressed.
"Every sibling dialog in the same directory includes a
DialogTitle." — Netero
🤖 This review was automatically generated with Coder Agents.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e58f1168a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
/coder-agents-review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Well-structured search dialog with thorough story coverage and clean component decomposition. DEREM-1 and DEREM-2 from round 1 both addressed correctly.
2 P2, 11 P3, 7 Nit. The two P2s are a query normalization bug (duplicate title: filters rejected by backend) and a rendering issue (literal backticks in help text). Most P3s cluster around the search query normalizer and the dialog's debounce/keyboard behavior.
The searchQuery.ts normalizer has two edge cases where the frontend and backend disagree about the wire format (DEREM-3, DEREM-12). Both are real user paths: someone refining a title: search, or searching for a title with quotes. The placeholderData: keepPreviousData pattern (DEREM-11) follows the same approach used in UserAutocomplete for the same problem.
Process note: the initial commit shipped stories that could not pass (Radix dialog portals render to document.body, not the Storybook canvas). This was caught and fixed in a follow-up commit, but suggests the stories were written without being executed. Not a current code defect, so not raised as a finding, but worth noting for future workflow.
"The normalizer's purpose is to bridge user input to the backend's strict key:value grammar. It already collects title values into titleTerms and merges them when bare text is present. But the merge only fires when hasFallbackTitle is true. Two explicit title: filters without bare text skip the merge entirely." — Hisoka
🤖 This review was automatically generated with Coder Agents.
Addresses unresolved findings from the latest review pass: - Strip quotes from bare title text instead of escaping them, since the backend search-query parser has no backslash-escape handling (DEREM-12). - Add doc comment to normalizeChatSearchInput (DEREM-13). - Rename supportedChatSearchFilterKeys to passthroughChatSearchFilterKeys to clarify why 'title' is excluded (DEREM-16). - Rename hasFallbackTitle to hasBareTitleText (DEREM-19). - Add test for trailing-colon filter behavior (DEREM-17). - Use keepPreviousData to avoid skeleton flash between debounced keystrokes (DEREM-11). - Rename highlightedChatIndex state to selectedChatIndex; rename clamped variable to safeSelectedChatIndex to keep the concept under one name (DEREM-14). - Extract getChatDisplayConfig helper in statusConfig.ts; use in both ChatSearchResults and ChatTreeNode so the icon derivation isn't duplicated (DEREM-10). - Extract RESULTS_AREA_MIN_HEIGHT constant (DEREM-20). - Pluralize result count: '1 result' vs 'N results' (DEREM-18). - Drop the redundant space-y-1 wrapper around the search input (DEREM-21).
Resolved conflict in coderd/searchquery/search_test.go by taking main's version. The chat search backend (#25391) is now in main, so the PR branch's older copies of backend files (coderd/searchquery, coderd/exp_chats, coderd/database/queries, coderd/apidoc, docs/reference/api/chats.md) were reset to main's versions to avoid divergence with the merged backend.
closes CODAGT-422