refactor: replace MCP logs filter dropdown with dedicated sidebar#2745
refactor: replace MCP logs filter dropdown with dedicated sidebar#2745
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 8 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdded a new MCPFilterSidebar and restructured the MCP logs page into a sidebar + main flex layout; removed popover/command multi-select filters and RTK-query fetching from the main filters view, added predefined time-period presets, and adjusted table layout and cell sizing to flex-based scrolling. Changes
Sequence Diagram(s)sequenceDiagram
participant Page as MCP Logs Page
participant Sidebar as MCPFilterSidebar
participant API as FilterData API
participant Table as Logs Table
Page->>Sidebar: render with current filters
Sidebar->>Sidebar: user opens section (e.g., Tools)
Sidebar->>API: fetch filter options (useGetMCPLogsFilterDataQuery) [lazy when opened]
API-->>Sidebar: return options
Sidebar->>Page: onFiltersChange(updatedFilters)
Page->>Table: re-render with updated filters (query params)
Table->>API: fetch logs with filters
API-->>Table: return logs
Table-->>Page: display updated rows
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
16506cc to
cfda152
Compare
d3fa3f7 to
7100207
Compare
cfda152 to
6248cbc
Compare
7100207 to
8e91edd
Compare
6248cbc to
4a3be3b
Compare
8e91edd to
df190ec
Compare
0a5dab0 to
a152ed2
Compare
Confidence Score: 4/5Safe to merge after resolving the two open thread issues (content_search dropped on reset, userModifiedTimeRange set on every filter change); the dead-prop cleanup is minor. Two P1-level bugs from prior review threads remain open: handleReset silently clears the search box and every sidebar filter change permanently suppresses time-range auto-refresh. The only new finding is a P2 dead-prop cleanup. Score is 4 rather than 3 because the issues don't break primary data fetching or navigation, only the reset UX and the stale-time-range refresh. ui/components/filters/mcpFilterSidebar.tsx (handleReset bugs) and ui/app/workspace/mcp-logs/page.tsx (setFilters userModifiedTimeRange guard) Important Files Changed
Reviews (6): Last reviewed commit: "improvement: MCP page filter UI revamp" | Re-trigger Greptile |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/app/workspace/mcp-logs/views/mcpLogsTable.tsx (1)
41-42:⚠️ Potential issue | 🟠 MajorRemove unused
fetchLogsandfetchStatsprops.These props are declared in the interface and passed by the parent component but never destructured or used within the component.
Required changes
Remove from
DataTablePropsinterface:- fetchLogs: () => Promise<void>; - fetchStats: () => Promise<void>;Remove from parent component (
page.tsxlines 566-567):- fetchLogs={fetchLogs} - fetchStats={fetchStats}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/mcp-logs/views/mcpLogsTable.tsx` around lines 41 - 42, DataTableProps declares unused props fetchLogs and fetchStats; remove those two properties from the DataTableProps interface and delete the corresponding props passed from the parent component where the DataTable is instantiated (the place currently passing fetchLogs and fetchStats). Ensure no references to fetchLogs or fetchStats remain in mcpLogsTable.tsx (component) or in the parent component that instantiates it (remove the props from the JSX/props object).
🧹 Nitpick comments (4)
ui/components/filters/mcpFilterSidebar.tsx (3)
189-195: Missingdata-testidon search input.The search input in
SearchableCheckboxListlacks adata-testidattribute for E2E testing.Suggested fix
function SearchableCheckboxList({ items, isSelected, onToggle, placeholder = "Search...", inputRef, + testId, }: { items: { key: string; label: string }[]; isSelected: (key: string) => boolean; onToggle: (key: string) => void; placeholder?: string; inputRef?: Ref<HTMLInputElement>; + testId?: string; }) { ... <Input ref={inputRef} value={query} onChange={(e) => setQuery(e.target.value)} placeholder={placeholder} className="h-8 border-0 text-xs" + data-testid={testId ? `${testId}-search-input` : undefined} />As per coding guidelines:
ui/**/*.{tsx,ts}: Add new interactive UI elements withdata-testidattributes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/filters/mcpFilterSidebar.tsx` around lines 189 - 195, The SearchableCheckboxList search Input (props: ref=inputRef, value=query, onChange=setQuery, placeholder=placeholder) is missing a data-testid for E2E tests; add a data-testid prop (e.g. data-testid="mcp-filter-search-input" or similar consistent name) to the Input component in mcpFilterSidebar.tsx so the input can be reliably selected in tests.
148-153: Missingdata-testidon checkbox items.The
CheckboxFilterItemcomponent renders interactive checkboxes withoutdata-testidattributes. Consider adding atestIdprop to support E2E testing.Suggested fix
function CheckboxFilterItem({ label, checked, onCheckedChange, labelClassName, + testId, }: { label: string; checked: boolean; onCheckedChange: (checked: boolean) => void; labelClassName?: string; + testId?: string; }) { return ( - <label className="hover:bg-muted/50 flex cursor-pointer items-center gap-2.5 px-3 py-2 text-sm"> - <Checkbox checked={checked} onCheckedChange={onCheckedChange} /> + <label className="hover:bg-muted/50 flex cursor-pointer items-center gap-2.5 px-3 py-2 text-sm" data-testid={testId}> + <Checkbox checked={checked} onCheckedChange={onCheckedChange} data-testid={testId ? `${testId}-checkbox` : undefined} /> <span className={cn("truncate", labelClassName)}>{label}</span> </label> ); }As per coding guidelines:
ui/**/*.{tsx,ts}: Add new interactive UI elements withdata-testidattributes. Based on learnings, data-testid additions can be batched in a dedicated PR with related E2E tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/filters/mcpFilterSidebar.tsx` around lines 148 - 153, CheckboxFilterItem is missing a data-testid which blocks E2E targeting; add an optional prop (e.g., testId?: string) to the CheckboxFilterItem component in mcpFilterSidebar.tsx, forward that prop into the rendered markup as a data-testid (for example data-testid={testId} on the outer <label> or data-testid={`${testId}-checkbox`} on the <Checkbox>), and update any callers to pass a meaningful testId where tests will target it; ensure prop typing is updated and default handling preserves existing behavior when testId is undefined.
47-50: Missingdata-testidon the Reset button.Per coding guidelines, interactive UI elements should include
data-testidattributes following the<entity>-<element>-<qualifier>pattern.Suggested fix
- <Button variant="outline" size="sm" className="text-muted-foreground h-7 px-2 text-xs" onClick={handleReset}> + <Button variant="outline" size="sm" className="text-muted-foreground h-7 px-2 text-xs" onClick={handleReset} data-testid="mcp-filter-reset-btn"> <RotateCcw className="size-3" /> Reset </Button>As per coding guidelines:
ui/**/*.{tsx,ts}: Add new interactive UI elements withdata-testidattributes following the pattern:data-testid="<entity>-<element>-<qualifier>".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/filters/mcpFilterSidebar.tsx` around lines 47 - 50, The Reset Button is missing a data-testid; update the Button JSX (the Button that renders RotateCcw and calls handleReset) to include a data-testid attribute following the <entity>-<element>-<qualifier> pattern (for example data-testid="mcpFilter-button-reset" or "mcpFilter-reset-button"); add this attribute to the Button element so tests can reliably target it while leaving the onClick handler (handleReset) and icon (RotateCcw) unchanged.ui/app/workspace/mcp-logs/views/mcpLogsTable.tsx (1)
193-215: Status row always visible above data rows.This creates a permanent status indicator row (socket connection / live updates state) that appears above the actual log data. While this provides persistent feedback, it may add visual clutter since the status is already indicated by the live toggle button in the filter bar.
Consider whether this UX is intentional or if the status row should only appear when there's relevant information to show (e.g., socket disconnected).
Alternative: Only show status row when disconnected or helpful
- <TableRow className="hover:bg-transparent"> - <TableCell colSpan={columns.length} className="h-12 text-center"> - <div className="flex items-center justify-center gap-2"> - {!isSocketConnected ? ( - <> - <X className="h-4 w-4" /> - Not connected to socket, please refresh the page. - </> - ) : liveEnabled ? ( - <> - <RefreshCw className="h-4 w-4 animate-spin" /> - Listening for logs... - </> - ) : ( - <> - <Pause className="h-4 w-4" /> - Live updates paused - </> - )} - </div> - </TableCell> - </TableRow> + {!isSocketConnected && ( + <TableRow className="hover:bg-transparent"> + <TableCell colSpan={columns.length} className="h-12 text-center"> + <div className="flex items-center justify-center gap-2"> + <X className="h-4 w-4" /> + Not connected to socket, please refresh the page. + </div> + </TableCell> + </TableRow> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/mcp-logs/views/mcpLogsTable.tsx` around lines 193 - 215, The persistent status TableRow (using TableRow, TableCell and icons X/RefreshCw/Pause) is always rendered above log rows; change the render so the status row only appears when relevant by wrapping the entire TableRow block in a conditional like: render it only when !isSocketConnected || !liveEnabled (i.e., socket disconnected or live updates paused) instead of always showing it, keeping the existing inner JSX (icons and messages) intact and using the existing columns length check for colSpan.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@ui/app/workspace/mcp-logs/views/mcpLogsTable.tsx`:
- Around line 41-42: DataTableProps declares unused props fetchLogs and
fetchStats; remove those two properties from the DataTableProps interface and
delete the corresponding props passed from the parent component where the
DataTable is instantiated (the place currently passing fetchLogs and
fetchStats). Ensure no references to fetchLogs or fetchStats remain in
mcpLogsTable.tsx (component) or in the parent component that instantiates it
(remove the props from the JSX/props object).
---
Nitpick comments:
In `@ui/app/workspace/mcp-logs/views/mcpLogsTable.tsx`:
- Around line 193-215: The persistent status TableRow (using TableRow, TableCell
and icons X/RefreshCw/Pause) is always rendered above log rows; change the
render so the status row only appears when relevant by wrapping the entire
TableRow block in a conditional like: render it only when !isSocketConnected ||
!liveEnabled (i.e., socket disconnected or live updates paused) instead of
always showing it, keeping the existing inner JSX (icons and messages) intact
and using the existing columns length check for colSpan.
In `@ui/components/filters/mcpFilterSidebar.tsx`:
- Around line 189-195: The SearchableCheckboxList search Input (props:
ref=inputRef, value=query, onChange=setQuery, placeholder=placeholder) is
missing a data-testid for E2E tests; add a data-testid prop (e.g.
data-testid="mcp-filter-search-input" or similar consistent name) to the Input
component in mcpFilterSidebar.tsx so the input can be reliably selected in
tests.
- Around line 148-153: CheckboxFilterItem is missing a data-testid which blocks
E2E targeting; add an optional prop (e.g., testId?: string) to the
CheckboxFilterItem component in mcpFilterSidebar.tsx, forward that prop into the
rendered markup as a data-testid (for example data-testid={testId} on the outer
<label> or data-testid={`${testId}-checkbox`} on the <Checkbox>), and update any
callers to pass a meaningful testId where tests will target it; ensure prop
typing is updated and default handling preserves existing behavior when testId
is undefined.
- Around line 47-50: The Reset Button is missing a data-testid; update the
Button JSX (the Button that renders RotateCcw and calls handleReset) to include
a data-testid attribute following the <entity>-<element>-<qualifier> pattern
(for example data-testid="mcpFilter-button-reset" or "mcpFilter-reset-button");
add this attribute to the Button element so tests can reliably target it while
leaving the onClick handler (handleReset) and icon (RotateCcw) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea0eb64b-7e2d-4b40-ac15-91526f9c83d3
📒 Files selected for processing (4)
ui/app/workspace/mcp-logs/page.tsxui/app/workspace/mcp-logs/views/filters.tsxui/app/workspace/mcp-logs/views/mcpLogsTable.tsxui/components/filters/mcpFilterSidebar.tsx
a152ed2 to
91818b7
Compare
2755ff1 to
bd7d209
Compare
91818b7 to
2dea7b9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (4)
ui/app/workspace/mcp-logs/views/filters.tsx (1)
111-117: Consider addingdata-testidfor the search input.Per coding guidelines, new interactive UI elements should have
data-testidattributes following the<entity>-<element>-<qualifier>pattern. Based on learnings, this could be deferred to a dedicated PR that also updates related e2e tests.🧪 Suggested data-testid addition
<Input type="text" className="!h-7 rounded-tl-none rounded-tr-sm rounded-br-sm rounded-bl-none border-none bg-slate-50 shadow-none outline-none focus-visible:ring-0 dark:bg-zinc-900" placeholder="Search MCP logs" value={localSearch} onChange={(e) => handleSearchChange(e.target.value)} + data-testid="mcp-logs-search-input" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/mcp-logs/views/filters.tsx` around lines 111 - 117, The search Input component rendering localSearch and using handleSearchChange should include a data-testid attribute to satisfy testing guidelines; add data-testid="mcp-logs-search-input" (following the <entity>-<element>-<qualifier> pattern) to the Input component instance that currently has props type, className, placeholder, value={localSearch} and onChange={(e) => handleSearchChange(e.target.value)} so e2e tests can reliably target it.ui/components/filters/mcpFilterSidebar.tsx (3)
41-51: Consider addingdata-testidattributes to key interactive elements.Per coding guidelines, new interactive UI elements should have
data-testidattributes. This applies to the Reset button here and various checkboxes/inputs throughout this file. Based on learnings, this could be addressed in a dedicated PR alongside e2e test updates.🧪 Example data-testid additions
<div className="bg-card flex h-full w-64 shrink-0 flex-col rounded-r-md"> {/* Header */} <div className="flex h-11 items-center justify-between border-b pr-3 pl-5"> <span className="text-sm font-semibold">Filters</span> {activeFilterCount > 0 && ( - <Button variant="outline" size="sm" className="text-muted-foreground h-7 px-2 text-xs" onClick={handleReset}> + <Button variant="outline" size="sm" className="text-muted-foreground h-7 px-2 text-xs" onClick={handleReset} data-testid="mcp-filter-reset-btn"> <RotateCcw className="size-3" /> Reset </Button> )} </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/filters/mcpFilterSidebar.tsx` around lines 41 - 51, Add data-testid attributes to all key interactive elements in mcpFilterSidebar.tsx: for the Reset Button rendered when activeFilterCount > 0 (the Button with onClick={handleReset}) add something like data-testid="filters-reset-button", and add descriptive data-testid values to each filter control (checkboxes, inputs, selects) used in this file—e.g., for checkbox components or inputs used inside functions/render blocks add data-testid values like "filter-{filterKey}-checkbox" or "filter-{filterKey}-input". Ensure each interactive component (Button, Checkbox, Input, Select) has a unique, stable data-testid string so e2e tests can target them reliably.
111-113: One-way sync fordefaultOpenis intentional but worth documenting.The
useEffectonly opens the section whendefaultOpenbecomes true but never closes it. This appears intentional (sections stay open once opened by user or by having active filters), but a brief comment would clarify the design intent.📝 Suggested comment
+ // Sync open state when defaultOpen becomes true (e.g., when filters become active). + // Intentionally one-way: we don't close the section if defaultOpen becomes false, + // allowing users to keep sections open after manual interaction. useEffect(() => { if (defaultOpen) setOpen(true); }, [defaultOpen]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/filters/mcpFilterSidebar.tsx` around lines 111 - 113, Add a short inline comment above the useEffect that references defaultOpen, setOpen, and the open state to explain the intentional one-way sync: that the effect only opens the panel when defaultOpen becomes true but deliberately does not close it on later changes so user-opened sections (or those activated by filters) remain open. Keep the comment concise and mention that this behavior is by design to prevent auto-closing after user interaction.
305-315: Consider memoizingisSelectedandtogglewithuseCallback.These functions are recreated on every render. While the performance impact is likely minimal given the component's scope, wrapping them in
useCallbackwould be more consistent with React best practices and prevent unnecessary re-renders of child components.♻️ Suggested memoization
- const isSelected = (name: string) => { + const isSelected = useCallback((name: string) => { const id = nameToId.get(name) || name; return (filters.virtual_key_ids || []).includes(id); - }; + }, [nameToId, filters.virtual_key_ids]); - const toggle = (name: string) => { + const toggle = useCallback((name: string) => { const id = nameToId.get(name) || name; const current = filters.virtual_key_ids || []; const next = current.includes(id) ? current.filter((v) => v !== id) : [...current, id]; onFiltersChange({ ...filters, virtual_key_ids: next }); - }; + }, [nameToId, filters, onFiltersChange]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/filters/mcpFilterSidebar.tsx` around lines 305 - 315, Wrap the isSelected and toggle functions with React's useCallback to avoid recreating them each render: import useCallback from 'react' and change isSelected to const isSelected = useCallback((name: string) => { const id = nameToId.get(name) || name; return (filters.virtual_key_ids || []).includes(id); }, [nameToId, filters.virtual_key_ids]); and change toggle to const toggle = useCallback((name: string) => { const id = nameToId.get(name) || name; const current = filters.virtual_key_ids || []; const next = current.includes(id) ? current.filter((v) => v !== id) : [...current, id]; onFiltersChange({ ...filters, virtual_key_ids: next }); }, [nameToId, filters, onFiltersChange]); ensure useCallback is imported and the dependency arrays reference nameToId, filters (or filters.virtual_key_ids) and onFiltersChange appropriately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ui/app/workspace/mcp-logs/views/filters.tsx`:
- Around line 111-117: The search Input component rendering localSearch and
using handleSearchChange should include a data-testid attribute to satisfy
testing guidelines; add data-testid="mcp-logs-search-input" (following the
<entity>-<element>-<qualifier> pattern) to the Input component instance that
currently has props type, className, placeholder, value={localSearch} and
onChange={(e) => handleSearchChange(e.target.value)} so e2e tests can reliably
target it.
In `@ui/components/filters/mcpFilterSidebar.tsx`:
- Around line 41-51: Add data-testid attributes to all key interactive elements
in mcpFilterSidebar.tsx: for the Reset Button rendered when activeFilterCount >
0 (the Button with onClick={handleReset}) add something like
data-testid="filters-reset-button", and add descriptive data-testid values to
each filter control (checkboxes, inputs, selects) used in this file—e.g., for
checkbox components or inputs used inside functions/render blocks add
data-testid values like "filter-{filterKey}-checkbox" or
"filter-{filterKey}-input". Ensure each interactive component (Button, Checkbox,
Input, Select) has a unique, stable data-testid string so e2e tests can target
them reliably.
- Around line 111-113: Add a short inline comment above the useEffect that
references defaultOpen, setOpen, and the open state to explain the intentional
one-way sync: that the effect only opens the panel when defaultOpen becomes true
but deliberately does not close it on later changes so user-opened sections (or
those activated by filters) remain open. Keep the comment concise and mention
that this behavior is by design to prevent auto-closing after user interaction.
- Around line 305-315: Wrap the isSelected and toggle functions with React's
useCallback to avoid recreating them each render: import useCallback from
'react' and change isSelected to const isSelected = useCallback((name: string)
=> { const id = nameToId.get(name) || name; return (filters.virtual_key_ids ||
[]).includes(id); }, [nameToId, filters.virtual_key_ids]); and change toggle to
const toggle = useCallback((name: string) => { const id = nameToId.get(name) ||
name; const current = filters.virtual_key_ids || []; const next =
current.includes(id) ? current.filter((v) => v !== id) : [...current, id];
onFiltersChange({ ...filters, virtual_key_ids: next }); }, [nameToId, filters,
onFiltersChange]); ensure useCallback is imported and the dependency arrays
reference nameToId, filters (or filters.virtual_key_ids) and onFiltersChange
appropriately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c4c41223-304f-49d9-8148-f73e54785bc7
📒 Files selected for processing (4)
ui/app/workspace/mcp-logs/page.tsxui/app/workspace/mcp-logs/views/filters.tsxui/app/workspace/mcp-logs/views/mcpLogsTable.tsxui/components/filters/mcpFilterSidebar.tsx
✅ Files skipped from review due to trivial changes (1)
- ui/app/workspace/mcp-logs/views/mcpLogsTable.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/app/workspace/mcp-logs/page.tsx
bd7d209 to
07f582c
Compare
2dea7b9 to
00ad036
Compare
07f582c to
06b4f24
Compare
00ad036 to
3776ea7
Compare
Merge activity
|
3776ea7 to
c67b0cd
Compare
06b4f24 to
08f1341
Compare
08f1341 to
ed795ad
Compare
c67b0cd to
8f1da20
Compare
The base branch was changed.

Summary
Redesigned the MCP logs page with a dedicated filter sidebar to improve usability and organization. The new layout provides better space utilization and a more intuitive filtering experience.
Changes
MCPFilterSidebarcomponent with collapsible filter sections for Status, Tool Names, Servers, and Virtual KeysType of change
Affected areas
How to test
Navigate to the MCP logs page and verify:
Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
Breaking changes
Related issues
Link related issues and discussions. Example: Closes #123
Security considerations
No security implications - this is a UI layout and filtering enhancement.
Checklist
docs/contributing/README.mdand followed the guidelines