fix(site): prevent chat search filter pills from overflowing the dialog#26095
Conversation
|
/coder-agents-review |
|
Chat: Review in progress | View chat deep-review v0.7.1 | Round 4 | Last posted: Round 4, 9 findings (4 P3, 4 Nit, 1 Note), APPROVE. Review Finding inventoryFindings
Contested and acknowledged(none) Round logRound 1Panel (Netero + 12 reviewers + 1 wildcard). 3 P3, 1 Nit, 1 Note new. 1 dropped (scope creep). Reviewed against 8883a79..e18953f. Round 2Churn guard: PROCEED. All 5 open findings addressed in 1e60da0. Netero: no new findings, all fixes verified. Panel (7 reviewers + Netero). 1 P3, 3 Nit new. Reviewed against 8883a79..1e60da0. Round 4Base rebased (new base: 69f9b0e). New content: IconInputAlignment story, h-7 icon centering wrapper. Netero: no findings. Panel (4 reviewers): no findings. APPROVE. Reviewed against 69f9b0e..0713b36. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
|
Codex Review: Didn't find any major issues. Swish! ℹ️ 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.
Good dual fix: the CSS wrapping/truncation chain (min-w-0 at three levels, flex-wrap, truncate) is textbook flex-overflow handling, and auto-quoting colon-containing values in the normalizer closes a real backend parsing hole. The rawKey preservation in getKeyValuePair shows care for the data flow, and the normalizedTokens + return-path fix (line 149: normalizedTokens.join(" ") instead of trimmedInput) closes a subtle bypass where standalone diff_url:https://... queries silently discarded the normalization.
3 P3, 1 Nit, 1 Note.
Process note: the commit subject ("fix: fix search ui filter pills") doubles the verb and names the location instead of the problem. Something like "fix: auto-quote URLs in chat search filters and fix pill overflow" captures both concerns. The PR body ("closes CODAGT-556") leaves reviewers reverse-engineering the problem from the diff.
"Revert every CSS change in ChatSearchInput.tsx and ChatSearchDialog.tsx, and the story still passes. It proves the data flow works. It does not prove the layout fix works." Bisky
🤖 This review was automatically generated with Coder Agents.
…dback - Quote passthrough filter values that contain spaces so normalization is idempotent. - Document why passthrough values with colons or spaces must be quoted. - Note why the title branch keeps the raw token before re-quoting. - Add a title attribute to the truncated filter pill for a hover tooltip. - Assert the diff URL pill and search box stay within the dialog in the story.
|
/coder-agents-review |
There was a problem hiding this comment.
All 5 R1 findings verified fixed in 1e60da0. The overflow containment assertion (CRF-1), space-quoting fix (CRF-2), title tooltip (CRF-3), doc comment (CRF-4), and asymmetry comment (CRF-6) are all correct and complete. The new addDefaultURLScheme helper and idempotency test are good additions.
1 P3, 3 Nit new this round.
Comment verbosity pattern: 4 of 5 new comments added in the fix commit restate mechanism visible from the code. Gon provided minimum drafts for each. The strongest examples are the doc comment on normalizePassthroughChatSearchFilter (line 87, first sentence restates output format) and the title-branch comment (line 138, "Keep the raw token here" restates push(token)). Minimum drafts:
searchQuery.ts:87:// The backend splits on unquoted whitespace and colons, so values containing either must be quoted.searchQuery.ts:138:// Title tokens are merged and re-quoted later, so normalizing here would be redundant.stories.tsx:499:// Guards the overflow fix (min-w-0/truncate/max-w-full); without it the pill spills outside the dialog.test.ts:39:// Normalization must be idempotent.
Process notes (repeat from R1): the PR title "fix: fix search ui filter pills" doubles the verb and names the location instead of the behavior. Consider fix(site): prevent filter pills from overflowing search dialog. The PR body ("closes CODAGT-556") gives no context without the ticket. Also, the commit subject for the scheme helper says "url schema"; the RFC 3986 term is "scheme."
"One assertion would nail the CRF-3 fix verification." Bisky
🤖 This review was automatically generated with Coder Agents.
- Assert the diff URL pill exposes its full value via the title tooltip. - Trim doc/test comments to the essential insight, dropping restated mechanics. - Rename keyValuePairs to passthroughFilters to match its normalized contents. - Document that rawKey preserves casing while key is lowercased for matching.
|
/coder-agents-review |
There was a problem hiding this comment.
All 10 findings from R1 and R2 verified fixed across commits 1e60da0 and c777d6f. Netero R3 scan clean. PR title and description improved per process feedback.
The final PR is clean: CSS overflow prevention with min-w-0/truncate/max-w-full at three nesting levels, auto-quoting for passthrough values containing colons or spaces (with addDefaultURLScheme for schemeless diff URLs), title tooltip on truncated pills, and test coverage exercising both the layout fix (getBoundingClientRect containment) and the normalization pipeline (idempotency, scheme prepending, mixed queries). 50% test density, proportional to the change.
No open findings.
🤖 This review was automatically generated with Coder Agents.
The input row grew to 28px (text-sm line height 1.5rem plus py-0.5), but the search and filter icons used a 2px top margin that no longer centered them on that row, leaving them floating above the vertically centered input text. Wrap each icon in an h-7 box that matches the input row height and centers the icon, while keeping the container top-aligned so the icons stay put when filter pills wrap onto multiple lines. Add a story asserting the icons remain vertically centered with the input.
|
/coder-agents-review |
There was a problem hiding this comment.
Re-approve after rebase and new content. The IconInputAlignment story with getBoundingClientRect vertical-centering assertions and the h-7 icon wrapper (replacing the previous mt-0.5 approach) are clean additions. The fixed-height wrapper is a more robust alignment strategy since it doesn't depend on margin values matching the input's line height.
All 10 prior findings remain resolved. No new findings from Netero or the panel (Bisky, Nami, Mafuuu, Kite). 58% test density.
No open findings.
🤖 This review was automatically generated with Coder Agents.
Remove comments that restated the adjacent code or test intent and trim the icon-alignment note. Keep only the comments documenting non-obvious behavior (the backend parser quoting rule and the h-7 magic number).
Long filter values (for example diff URLs) were widening the chat search
dialog instead of staying inside the search box, and some passthrough
filters did not round-trip cleanly through the backend query parser.
This fixes the filter pills so long values truncate inside the search box
(with the full value available via a hover tooltip), and normalizes
passthrough filters so values containing spaces or colons are quoted and
re-parse identically.
closes CODAGT-556