replace other InputGroup components#2830
Conversation
🦋 Changeset detectedLatest commit: 13b021d The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@claude --review and add a changeset |
|
TL;DR — Replaces ad-hoc search input patterns (absolutely positioned icons, manual padding classes) across five manage-UI components with the Key changes
Summary ｜ 6 files ｜ 14 commits ｜ base: Search inputs migrated to
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean migration from ad-hoc absolute-positioned icon patterns to the InputGroup compound component across all 5 identified locations. One medium-severity UX concern worth addressing before merge.
| {hasActiveFilters && ( | ||
| <InputGroupAddon align="inline-end"> | ||
| <Button variant="ghost" size="icon-sm" onClick={clearFilters}> | ||
| <X /> | ||
| </Button> | ||
| </InputGroupAddon> | ||
| )} |
There was a problem hiding this comment.
This X button calls clearFilters() which resets all filters (agent, output status, search text) — not just the search input. An icon-only X inside a search field conventionally means "clear this text field." The old code had a standalone "Clear Filters" button with a text label next to "Show Filters," which made the scope of the action explicit.
Either scope this button to only clear the search text (updateFilter('searchInput', '')) or keep a separate "Clear Filters" button outside the InputGroup for the broader reset action.
There was a problem hiding this comment.
The action is correctly scoped now — updateFilter('searchInput', '') only clears the search text. One small nit: the X button visibility is still gated on hasActiveFilters (any filter active), so it appears inside the search box even when searchInput is empty but another filter is set. Consider changing the condition to filters.searchInput so the button only shows when there's search text to clear.
agents-manage-ui/src/components/evaluation-jobs/evaluation-results-filters.tsx
Outdated
Show resolved
Hide resolved
agents-manage-ui/src/components/traces/conversation-stats/conversation-stats-card.tsx
Show resolved
Hide resolved
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Medium
🟠⚠️ Major (2) 🟠⚠️
Inline Comments:
- 🟠 Major:
test-case-filters.tsx:61X button inside search clears ALL filters instead of just search text - 🟠 Major:
evaluation-results-filters.tsx:76X button inside search clears ALL filters instead of just search text
Both issues stem from the same UX problem: the X button was moved from a separate "Clear Filters" button (outside the search box) to inline inside the InputGroup (inside the search box), but the clearFilters() behavior was kept. Users expect an X button inside a search field to clear only the search text, not unrelated dropdown filters.
💭 Consider (5) 💭
💭 1) nango-providers-grid.tsx:93, test-case-filters.tsx:56, evaluation-results-filters.tsx:71, conversation-stats-card.tsx:128, channel-defaults-section.tsx:254 Explicit align prop for Search icon
Issue: The Search icon InputGroupAddon doesn't specify align prop, relying on the default (inline-start).
Why: While functionally correct (default is inline-start), explicit is better than implicit for maintainability. The clear button addons correctly specify align="inline-end".
Fix: Add align="inline-start" to Search icon addons for consistency with the align="inline-end" pattern used for clear buttons.
✅ What Looks Good
- Clean migration from ad-hoc
Input+ absolute-positioned icons to the unifiedInputGroupcomponent system - Consistent use of
InputGroupAddonwithalign="inline-end"for clear buttons - Proper use of
InputGroupInputwhich delegates styling to the Input component - The
aria-invalidattribute is properly passed through inconversation-stats-card.tsx - Button sizes consistently use
size="icon-sm"for the X buttons
🚫 REQUEST CHANGES
Summary: The refactoring is clean and the InputGroup component API is being used correctly. However, there's a UX regression in two files where the clear button behavior changed unexpectedly. The X button inside the search input should only clear the search text, not all filters. This is a quick fix — just change clearFilters to () => updateFilter('searchInput', '') in the two affected files.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-frontend |
7 | 0 | 5 | 0 | 2 | 0 | 0 |
| Total | 7 | 0 | 5 | 0 | 2 | 0 | 0 |
agents-manage-ui/src/components/evaluation-jobs/evaluation-results-filters.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
PR Review Summary
(3) Total Issues | Risk: Low
🟡 Minor (3) 🟡
Inline Comments:
- 🟡 Minor:
test-case-filters.tsx:61Missing aria-label on clear button - 🟡 Minor:
evaluation-results-filters.tsx:76Missing aria-label on clear button - 🟡 Minor:
conversation-stats-card.tsx:133Missing aria-label on clear button
💭 Consider (2) 💭
💭 1) test-case-filters.tsx:49 InputGroup loses flex-1 behavior
Issue: The old wrapper had flex-1 allowing the input to grow within the flex container. The new InputGroup only has max-w-sm.
Why: On wider screens, the input will now stay at its natural width rather than expanding to fill available space.
Fix: If the input should grow, add flex-1 to InputGroup: <InputGroup className="flex-1 max-w-sm">.
💭 2) evaluation-results-filters.tsx:64 InputGroup loses flex-1 and min-w-[200px]
Issue: The old wrapper had flex-1 min-w-[200px] max-w-sm but the new InputGroup only has max-w-sm.
Why: The input may collapse below 200px on smaller viewports and won't grow to fill available space.
Fix: Consider adding flex-1 min-w-[200px] to match previous behavior.
💡 APPROVE WITH SUGGESTIONS
Summary: Clean refactoring to standardize on the InputGroup component! 🎉 The only issues are missing aria-label attributes on 3 clear buttons — channel-defaults-section.tsx correctly has aria-label="Clear search" but the other files are missing it. All three have 1-click suggestions attached. The flex-1 changes are likely intentional design decisions but worth confirming.
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-frontend |
6 | 0 | 2 | 0 | 3 | 0 | 1 |
pr-review-consistency |
3 | 0 | 0 | 0 | 0 | 0 | 3 |
| Total | 9 | 0 | 2 | 0 | 3 | 0 | 4 |
Note: Consistency reviewer findings were deduplicated with frontend reviewer (same aria-label issues).
Discarded (4)
| Location | Issue | Reason Discarded |
|---|---|---|
nango-providers-grid.tsx:87 |
Changed responsive breakpoint from max-w-md to sm:max-w-md | Likely intentional improvement for mobile UX (full-width search on small screens) |
test-case-filters.tsx:59-65 |
UX change: visible "Clear Filters" button moved to inline icon | This is an intentional design decision as part of the InputGroup standardization |
evaluation-results-filters.tsx:74-80 |
UX change: visible "Clear" button moved to inline icon | Same as above — intentional design decision |
conversation-stats-card.tsx:116 |
Search input moved inside CardTitle | Layout restructuring is intentional to accommodate the InputGroup pattern |
agents-manage-ui/src/components/evaluation-jobs/evaluation-results-filters.tsx
Outdated
Show resolved
Hide resolved
agents-manage-ui/src/components/traces/conversation-stats/conversation-stats-card.tsx
Outdated
Show resolved
Hide resolved
Ito Test Report ❌16 test cases ran. 1 failed, 15 passed. Overall, 16 test cases were executed with 15 passing and 1 failing, confirming stable behavior across InputGroup-driven search/filter/clear flows in Credentials Providers, Dataset Runs, Evaluation Results/Run Configs, Traces debounce and pagination, Slack channel filtering (including mobile), protected-route auth redirects, and clear-button idempotency, with malformed/injection-like inputs remaining inert and no crashes or unexpected resets observed. The single confirmed Medium defect is in Slack channel permission boundaries, where unauthorized channel-agent updates can show success feedback and optimistic UI changes despite backend payloads returning success:false on HTTP 200, causing misleading UX until a refresh reveals the change was not persisted. ❌ Failed (1)
🟠 Slack permission boundary blocks unauthorized channel configuration
Relevant code:
const workspace = await findWorkspaceConnectionByTeamId(teamId);
if (!workspace || !verifyTenantOwnership(c, workspace.tenantId)) {
logger.warn({ teamId }, 'Workspace not found or tenant mismatch');
return c.json({ success: false, configId: '' });
}
if (!response.ok) {
const error = await response.json().catch(() => ({ error: 'Unknown error' }));
throw new Error(error.error || 'Failed to set channel agent');
}
return response.json();
try {
await slackApi.setChannelDefaultAgent({
teamId,
channelId,
agentConfig: {
projectId: agent.projectId,
agentId: agent.id,
grantAccessToMembers,
},
channelName,
});
setChannels((prev) =>
prev.map((ch) =>
ch.id === channelId ? { ...ch, hasAgentConfig: true, agentConfig: config } : ch
)
);
toast.success(`#${channelName} now uses "${agent.name}"`);✅ Passed (15)
Commit: Tell us how we did: Give Ito Feedback |
|
Reviewed the updated code in |
|
Addressed review feedback on the Changes pushed ( The
|
|
Addressed review feedback on
Pushed in commit |
|
Verified all review feedback from @dimaMachina's "check now" requests across 4 threads:
All originally flagged issues (clear-all-filters UX regression + missing aria-labels) are resolved. No code changes needed. |
|
Verified that the accessibility issue flagged in the review has been addressed. The icon-only clear button at |
|
Checked the review comment from @dimaMachina on |
|
Verified the review feedback on |
|
@claude changeset please |
|
Claude finished @dimaMachina's task in 1m 5s —— View job Adding Changeset
Added changeset Pushed in commit |
Co-authored-by: Dimitri POSTOLOV <dimaMachina@users.noreply.github.com>
|
Acknowledged the author's clarification that |
Ito Test Report ❌11 test cases ran. 1 failed, 10 passed. Overall, 11 test cases were executed with 10 passing and 1 failing, indicating the InputGroup migration and related search/filter flows are largely stable with no confirmed production defects in the covered providers, evaluations, traces, Slack, whitespace-normalization, zero-results, and XSS-safety scenarios. The key issue was one Medium bug introduced in dataset run details on mobile (390x844), where a non-wrapping filter header row can overflow or clip the search and Show/Hide Filters controls, while authorization boundaries for restricted Slack channel edits correctly blocked unauthorized changes without persistence. ❌ Failed (1)
🟠 Mobile viewport layout for updated search groups
Relevant code:
<div className="flex items-center gap-3">
<InputGroup className="max-w-sm">
<InputGroupInput
type="text"
placeholder="Search test cases..."
value={filters.searchInput || ''}
onChange={(e) => updateFilter('searchInput', e.target.value)}
/>
<InputGroupAddon>
<Search />
</InputGroupAddon>
{filters.searchInput && (
<InputGroupAddon align="inline-end">
<Button variant="ghost" size="icon-sm" onClick={() => { updateFilter('searchInput', ''); }} aria-label="Clear search">
<X />
</Button>
</InputGroupAddon>
)}
</InputGroup>
<Button variant="outline" size="sm" onClick={() => setIsExpanded(!isExpanded)} className="gap-2">
<Filter className="h-4 w-4" />
{isExpanded ? 'Hide Filters' : 'Show Filters'}
</Button>
</div>
className={cn(
'group/input-group relative flex w-full items-center rounded-md border border-input shadow-xs transition-[color,box-shadow] outline-none dark:bg-input/30',
'h-9 min-w-0 has-[>textarea]:h-auto',
<div className="flex items-center justify-between gap-3 mb-3 flex-wrap">
<div className="flex gap-3">
<ChannelFilter ... />
<ChannelFilter ... />
<ChannelFilter ... />
</div>
<InputGroup className="max-w-md">
<InputGroupInput
placeholder="Search channels..."
value={channelSearchQuery}
onChange={(e) => onSearchQueryChange(e.target.value)}
/>✅ Passed (10)
Commit: Tell us how we did: Give Ito Feedback |
1 participant
based on #2810 (comment)