ui: refine MCP Server tab layout and alignment#12957
Draft
rodrigosnader wants to merge 2 commits intomainfrom
Draft
ui: refine MCP Server tab layout and alignment#12957rodrigosnader wants to merge 2 commits intomainfrom
rodrigosnader wants to merge 2 commits intomainfrom
Conversation
- Wrap install/JSON content in dashed bordered container - Lift Edit Tools modal state to McpFlowsSection so the button can sit next to the Flows/Tools label - Align first chip row with Auto install/JSON tabs - Center Auth row vertically - Drop redundant top margin in McpAutoInstallContent ToolsComponent gains optional hideButton, open and setOpen props for external trigger control.
Contributor
WalkthroughThis PR refactors modal state management for MCP tool editing by introducing controlled/uncontrolled prop patterns to ChangesMCP Tools Modal State & Layout Refinement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 4 warnings)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
❌ Your project check has failed because the head coverage (50.21%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #12957 +/- ##
==========================================
- Coverage 52.99% 52.90% -0.09%
==========================================
Files 2030 2031 +1
Lines 183826 184013 +187
Branches 26169 26231 +62
==========================================
- Hits 97415 97359 -56
- Misses 85312 85546 +234
- Partials 1099 1108 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/frontend/src/pages/MainPage/pages/homePage/components/McpFlowsSection.tsx (1)
55-68: 💤 Low value
button_description="Edit Tools"is dead code whenhideButtonistrue.
ToolsComponentonly usesbutton_descriptioninside the!hideButton && (...)block. SincehideButtonis alwaystruehere, this prop is never read.🧹 Proposed cleanup
<ToolsComponent value={flowsMCPData} title="MCP Server Tools" description="Select tools to add to this server" handleOnNewValue={handleOnNewValue} id="mcp-server-tools" - button_description="Edit Tools" editNode={false} isAction disabled={false} hideButton open={isModalOpen} setOpen={setIsModalOpen} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/pages/MainPage/pages/homePage/components/McpFlowsSection.tsx` around lines 55 - 68, ToolsComponent is being passed button_description="Edit Tools" while hideButton={true}, so that prop is dead; remove the unused prop from the ToolsComponent invocation in McpFlowsSection (i.e., delete the button_description="Edit Tools" attribute) to clean up dead code, or alternatively make hideButton configurable if you actually need the button; ensure changes reference the ToolsComponent props (button_description, hideButton) and update any related prop typings if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/frontend/src/components/core/parameterRenderComponent/types.ts`:
- Around line 62-64: The open/setOpen pair must be required together or not at
all; update the prop type that currently declares hideButton?: boolean; open?:
boolean; setOpen?: (open: boolean) => void to a discriminated union so consumers
cannot pass only one of open or setOpen. Replace the separate optional fields
with a combined type such as hideButton?: boolean plus a union for controlled vs
uncontrolled (controlled branch requires both open and setOpen, uncontrolled
branch forbids both), referencing the existing symbols open and setOpen in the
union so the compiler enforces they come as a pair.
---
Nitpick comments:
In
`@src/frontend/src/pages/MainPage/pages/homePage/components/McpFlowsSection.tsx`:
- Around line 55-68: ToolsComponent is being passed button_description="Edit
Tools" while hideButton={true}, so that prop is dead; remove the unused prop
from the ToolsComponent invocation in McpFlowsSection (i.e., delete the
button_description="Edit Tools" attribute) to clean up dead code, or
alternatively make hideButton configurable if you actually need the button;
ensure changes reference the ToolsComponent props (button_description,
hideButton) and update any related prop typings if necessary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 61dbd3e5-cd43-4b5c-a4b9-b6fd2edad633
📒 Files selected for processing (6)
src/frontend/src/components/core/parameterRenderComponent/components/ToolsComponent/index.tsxsrc/frontend/src/components/core/parameterRenderComponent/types.tssrc/frontend/src/pages/MainPage/pages/homePage/components/McpAuthSection.tsxsrc/frontend/src/pages/MainPage/pages/homePage/components/McpAutoInstallContent.tsxsrc/frontend/src/pages/MainPage/pages/homePage/components/McpFlowsSection.tsxsrc/frontend/src/pages/MainPage/pages/homePage/components/McpServerTab.tsx
Comment on lines
+62
to
+64
| hideButton?: boolean; | ||
| open?: boolean; | ||
| setOpen?: (open: boolean) => void; |
Contributor
There was a problem hiding this comment.
open and setOpen are independently optional — partial usage silently breaks the modal.
Because these two props are separate ?: fields, TypeScript permits passing only open or only setOpen. Both partial cases silently malfunction at runtime:
openprovided,setOpenomitted →setIsModalOpen = setInternalOpen; closing the modal updatesinternalOpenbutisModalOpen = open ?? internalOpenalways returns the externalopenvalue, so the modal is permanently stuck open.setOpenprovided,openomitted →isModalOpen = internalOpen; every call tosetIsModalOpengoes to the external setter but never updatesinternalOpen, so the modal never opens or closes from the component's perspective.
Constrain the type so both must be present together or absent together:
🛡️ Proposed fix — enforce controlled/uncontrolled as a unit
export type ToolsComponentType = {
description: string;
title: string;
icon?: string;
button_description?: string;
isAction?: boolean;
template?: APITemplateType;
hideButton?: boolean;
- open?: boolean;
- setOpen?: (open: boolean) => void;
-};
+} & (
+ | { open?: never; setOpen?: never }
+ | { open: boolean; setOpen: (open: boolean) => void }
+);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontend/src/components/core/parameterRenderComponent/types.ts` around
lines 62 - 64, The open/setOpen pair must be required together or not at all;
update the prop type that currently declares hideButton?: boolean; open?:
boolean; setOpen?: (open: boolean) => void to a discriminated union so consumers
cannot pass only one of open or setOpen. Replace the separate optional fields
with a combined type such as hideButton?: boolean plus a union for controlled vs
uncontrolled (controlled branch requires both open and setOpen, uncontrolled
branch forbids both), referencing the existing symbols open and setOpen in the
union so the compiler enforces they come as a pair.
Contributor
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
When the consumer renders its own trigger button (hideButton=true), the empty state now shows just a subtle left-aligned message instead of the dashed bordered box with a redundant Add button. Renames the empty-state copy from "actions" to "tools" to match the MCP Server tab terminology.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Refines the MCP Server tab layout in the home page so the two halves are visually aligned and the install content area is delimited:
mt-4inMcpAutoInstallContentthat was making the dashed box's top padding visually larger than its bottom paddingToolsComponent change
To let
McpFlowsSectionrender its own Edit Tools button (next to the section label) while still triggering the same modal,ToolsComponentnow accepts:hideButton?: boolean— skips rendering its internal absolute-positioned Edit buttonopen?: boolean/setOpen?: (open: boolean) => void— optional controlled modal stateWhen the new props are omitted, behavior is unchanged. The node parameter renderer (the other consumer) is untouched.
Screenshots
Before:
After:
Test plan
McpFlowsSection,McpServerTab,ToolsComponent,McpAuthSection,McpAutoInstallContent— 29 tests passingtoolsparameter renderer in nodes still looks the same (other consumer ofToolsComponent)