feat: update search dialog with filter pills and default view - #25753
Conversation
c70537a to
d6eab9b
Compare
tracyjohnsonux
changed the title
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.5.0 | Round 6 | Last posted: Round 6, 30 findings (1 P1, 9 P2, 17 P3, 1 P4, 2 Nit), APPROVE. Review Finding inventoryFindings
Contested and acknowledgedCRF-3 (P3, ChatSearchDialog.tsx:52) - SearchFilter type duplicated
CRF-15 (P3, ChatSearchDialog.tsx:237) - freeText destroyed on parameterized filter entry
CRF-21 (P4, ChatSearchInput.tsx:52) - Filter pills hand-roll Badge+dismiss
CRF-22 (Nit, ChatSearchDialog.tsx:20) - defaultValue naming
CRF-25 (P2, ChatSearchDialog.tsx:98) - Auto-focus drops keystrokes
CRF-30 (P3, ChatSearchDialog.tsx:36) - Independent sources of truth
Round logRound 1Netero-only. 1 P1, 1 P2, 3 P3. Reviewed against 5991a2c..d6eab9b. Round 2Churn guard overridden (BLOCKED->PROCEED). Panel: 13 reviewers. 5 P2, 9 P3, 1 Nit, 1 P4 new. Reviewed against 5991a2c..7c6e9a5. Round 3BLOCKED. CRF-3 and CRF-8 fixed. 16 findings silent. Reviewed against 5991a2c..1889e45. Round 4Churn guard overridden (BLOCKED->PROCEED). Panel: 4 reviewers (Bisky, Mafuuu, Pariston, Knov). 3 P2, 5 P3 new. Reviewed against 5991a2c..5644dad. Round 5Churn guard overridden (BLOCKED->PROCEED): 7/9 findings fixed, 1 contested (CRF-25), 1 accepted (CRF-30), CRF-5 still open since R1. Panel: 3 reviewers (Bisky, Pariston, Chopper). CRF-25 closed by panel (2/3 accept). APPROVE. Reviewed against 5991a2c..9d31166. Round 6CRF-5 fixed. PR significantly restructured (+398/-147, 14 files). New focusInputOnOpen prop, repeatable filters, keyboard nav for default view. Reviewed against 5991a2c..bc83184. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
First-pass review (Netero). These are mechanical findings from the first-pass scanner. The full review panel has not yet reviewed this PR; it will run after these are addressed.
1 P1, 1 P2, 3 P3.
444 new lines of interactive UI with zero new stories is the main blocker. The Settings button no-op and the keyboard navigation gap in the default view are worth fixing before the panel digs into design.
"The Settings button appears functional but does nothing the user can observe beyond dismissing the dialog." - Netero
site/src/pages/AgentsPage/components/ChatsSidebar/dialogs/ChatSearchDialog.tsx:188
P3 [CRF-5] Keyboard navigation non-functional for recent chats in default view.
resultCount derives from searchQuery.data?.length ?? 0. When no query is active (default view), the query is disabled so resultCount = 0. The ArrowDown/ArrowUp handler returns early when resultCount === 0 (line 306). The DefaultView renders recent chats in a <div role="listbox"> and passes selectedChatIndex, but the index is never set by keyboard.
Additionally, aria-controls on the combobox input is undefined when hasResults is false (ChatSearchInput.tsx:87), creating a disconnect: a listbox exists in the DOM but the combobox doesn't reference it. (Netero)
🤖
🤖 This review was automatically generated with Coder Agents.
e0d5540 to
7c6e9a5
Compare
|
/coder-agents-review |
7c6e9a5 to
6418bc0
Compare
|
/coder-agents-review |
6418bc0 to
a4cff2b
Compare
|
/coder-agents-review |
a4cff2b to
4542aa6
Compare
|
/coder-agents-review |
4542aa6 to
1889e45
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
The structural fix for CRF-2 (Link-by-construction instead of button+callback) is the right approach, and the five new stories exercise real interaction flows. The filter state model is clean, and the buildQuery/queryFreeText split that prevents incomplete filter values from leaking into title search is well-designed.
5 P2, 9 P3, 1 Nit.
The P2s cluster around two themes: the filter dropdown lifecycle (opens on focus, never closes during typing, covers results) and test coverage gaps where stories verify DOM without checking the API contract. The useDebouncedValue(effectiveFilters) infinite re-render and the unquoted buildQuery values are independent bugs with straightforward fixes.
CRF-5 (keyboard navigation for default view, P3 from R1) and CRF-6 (deleted Radix ScrollArea comment, P3 from R2 Netero) remain open.
"If
buildQuerysilently dropped filters or produced malformed queries, every new story would still pass because they only assert on pill DOM presence." - Bisky
🤖 This review was automatically generated with Coder Agents.
There was a problem hiding this comment.
CRF-3 (SearchFilter type deduplication) and CRF-8 (filter dropdown covering results) are fixed in this push. The toggle button replacing onFocus is a clean approach.
16 findings from the panel review remain unaddressed with no author response. Further review is blocked until the author responds or pushes fixes.
Open P2s (4): CRF-7 (ARIA/focus on dialog open), CRF-9 (filter stories missing API assertions), CRF-10 (buildQuery unquoted values), CRF-11 (useDebouncedValue infinite re-render).
Open P3s (7): CRF-5 (keyboard nav default view), CRF-6 (deleted ScrollArea comment), CRF-12 (DefaultView story no clicks), CRF-13 (Enter commit untested), CRF-14 (no combined filter+text story), CRF-15 (freeText destruction), CRF-16 (onSelectChat naming).
Open P3s (3): CRF-17 (FilterDropdown raw buttons), CRF-18 (narration comments), CRF-20 (ScrollArea config duplication).
Open Nit/P4 (3): CRF-19 (story comments), CRF-21 (Badge/TagInput reuse), CRF-22 (defaultValue naming).
🤖 This review was automatically generated with Coder Agents.
1889e45 to
5644dad
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
The author addressed 13 of 17 open findings in one push. All five P2s from R2 are fixed (ARIA focus, dropdown lifecycle, API assertions, unquoted values, infinite re-render), along with 8 P3s including the onSelectChat rename, FilterDropdown Button migration, comment cleanup, ScrollArea deduplication, and three new stories (Enter commit, combined filter+text). The fixes are well-crafted.
3 P2, 5 P3 new.
The P2s center on a regression from the CRF-8 fix: removing onFocus from the input fixed the dropdown-covers-results problem but broke all filter stories that click the input to open the dropdown. Six of seven filter stories are unrunnable and the CRF-9/13/14 fixes have never executed. The auto-focus change (CRF-25) is related: focus on the dialog container drops keystrokes, breaking the Cmd+K typing workflow, and the stated rationale (quick action visibility) doesn't hold because DefaultView renders based on query state, not focus state.
CRF-5 (keyboard navigation for default view, P3 from R1) remains open.
"When a user opens the dialog via Cmd+K to search, keystrokes are silently dropped. Focus is on the dialog container div, which doesn't accept text input." - Pariston
🤖 This review was automatically generated with Coder Agents.
5644dad to
9d31166
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
All R4 fixes verified. The stories now use the toggle button, TypedFilterAutoDetection covers the auto-detection path, stale debouncedFreeText is guarded, duplicate filter keys are consumed, internal quotes are stripped, and recent chat items are asserted.
Across 5 rounds, 30 findings raised: 24 fixed, 5 accepted/closed, 1 open (CRF-5, P3).
CRF-25 (auto-focus design choice) is closed by panel consensus. Pariston (original raiser) accepted it as mild UX friction. The author's intent is documented and the design has survived two rounds of challenge.
CRF-5 (P3, keyboard navigation for default view) remains open since R1. The DefaultView renders a role="listbox" but ArrowDown/ArrowUp can't reach it because resultCount is 0 when no query is active. Low severity for this PR; a follow-up could wire recentChats.length as the navigable count when !hasQuery and set aria-controls={listboxId} when the default view is showing. This needs a human decision on priority.
The panel noted a few P3 test coverage gaps worth considering: TypedFilterAutoDetection doesn't assert the API query contract (Bisky), no story exercises X-button pill removal or Escape-to-close-dropdown (Bisky), and only 1 of 4 quick action links is click-tested (Chopper). These are incremental improvements, not blockers.
"The problem is correctly understood, the solution is proportional, and the query building is at the right level." - Pariston
🤖 This review was automatically generated with Coder Agents.
jaaydenh
changed the title
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ce54544e7
ℹ️ 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".
|
@codex review |
|
/coder-agents-review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc83184ae2
ℹ️ 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".
There was a problem hiding this comment.
CRF-5 (keyboard navigation for default view, open since R1) is fixed. selectableChats falls through to recentChats when no query is active, ArrowDown/ArrowUp navigate, and aria-controls is connected. The RecentChatKeyboardNavigation story exercises the full path including Enter-to-navigate.
The focusInputOnOpen redesign (CRF-25) is clean: desktop auto-focuses the input (Radix default), mobile focuses the dialog container. The OpenedFromSearchButton story verifies the mobile path.
Across 6 rounds, 30 findings raised: 25 fixed, 5 accepted/closed. All resolved.
Three P3s from this round's panel (not blockers):
-
REPEATABLE_FILTER_KEYSis half-wired: the typed-filter path (line 315) allows duplicatepr_statusvalues, but the dropdown'saddFilterguard (line 246) andFilterDropdowndisabled state block it. The two interaction paths disagree on whetherpr_statusis repeatable. (Bisky, Mafuuu, Pariston, all P3) -
Boolean filter clicks are debounced 500ms before the search fires. Clicking "Unread" shows the pill immediately but results stay on the default view for half a second. The debounce is designed for text keystroke batching; a discrete filter click could bypass it. (Pariston P3)
-
DefaultViewWithQuickActionsstory verifies navigation link text but never clicks Settings, Personal Skills, or View usage to verify routes or dismiss behavior. (Bisky P3)
"This is a clean solution to a genuinely tricky focus problem." - Pariston, on the focusInputOnOpen redesign
🤖 This review was automatically generated with Coder Agents.
752311b to
0a347ed
Compare
aec2450 to
bbdebc7
Compare
…ls and default view Update the ChatSearchDialog with four interaction states: 1. Default (empty): quick actions (New chat, Settings) + recent chats list 2. Focused: filter-by dropdown with clickable chips (Unread, Archived, PR status, Diff URL) 3. Active search: filter pills and/or freeform text with debounced live results showing "N results" count 4. Parameterized filter: incomplete pill (dashed border) for filters needing a value (e.g. pr_status:) Filter pills and freeform text are combinable. The search input now renders filter pills inline, and Backspace on empty text removes the last pill. Changes: - ChatSearchDialog: manages structured filter state, renders filter dropdown, passes recent chats and quick action callbacks - ChatSearchInput: renders filter pills inline with the text input - ChatSearchResults: default view shows quick actions + recent chats instead of the old help text - ChatsSidebar: passes recentChats, onNewChat, onOpenSettings to dialog
bbdebc7 to
48ca5c3
Compare
2 participants
Update the ChatSearchDialog with four interaction states:
pr_status:); Space or Enter commits the value into a solid pillFilter pills and freeform text are combinable. Backspace on empty text removes the last pill. The filter dropdown overlays content below and dismisses on blur or Escape.
Changes:
ChatSearchDialog.tsx: manages structured filter state with separateincompleteFilterKeytracking, renders filter dropdown, passes recent chats and quick action callbacksChatSearchInput.tsx: renders completed filter pills (solid border, x dismiss) and incomplete pills (dashed border) inline with the text inputChatSearchResults.tsx: default view shows quick actions + recent chats instead of the old help text; "No matching chats" is centered in the modalChatsSidebar.tsx: passesrecentChats,onNewChat,onOpenSettingsto the dialogImplementation notes
chatSearch()API andnormalizeChatSearchInput()normalizerfiltersarray for committed filters +incompleteFilterKeystring for the parameterized filter being typed;freeTextserves dual purpose (filter value when incomplete key is set, otherwise freeform search)top-full) below the search input, dismisses via containeronBlurwithcontains(relatedTarget)checkLinkcomponents that close the dialog on click