-
Notifications
You must be signed in to change notification settings - Fork 654
Fix connection field sizing and add URL error hints #587
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the HTTP Local connection command UI to stay visible and provide clearer feedback when the URL is invalid, by changing the command field content, enabling/disabling styles and buttons, and adding error styling for hints and section layout. Class diagram for updated McpConnectionSection HTTP Local behaviorclassDiagram
class McpConnectionSection {
- httpServerCommandField
- httpServerCommandSection
- httpServerCommandHint
- copyHttpServerCommandButton
+ UpdateHttpServerCommandDisplay()
}
class HttpServerCommandField {
+ string value
+ string tooltip
}
class HttpServerCommandSection {
+ EnableInClassList(className bool)
}
class HttpServerCommandHint {
+ string text
+ AddToClassList(className)
+ RemoveFromClassList(className)
}
class CopyHttpServerCommandButton {
+ SetEnabled(isEnabled)
}
McpConnectionSection --> HttpServerCommandField : uses
McpConnectionSection --> HttpServerCommandSection : uses
McpConnectionSection --> HttpServerCommandHint : uses
McpConnectionSection --> CopyHttpServerCommandButton : uses
Flow diagram for HTTP Local command UI update on URL validationflowchart TD
A_start[Start UpdateHttpServerCommandDisplay] --> B_checkUrl[Check isLocalHttpUrl]
B_checkUrl -->|false| C_invalid[Set command field to <Invalid Localhost URL>]
C_invalid --> D_setTooltip[Set tooltip to explain non local address]
D_setTooltip --> E_markSectionInvalid[Enable invalid-url class on command section]
E_markSectionInvalid --> F_hintExists{httpServerCommandHint != null}
F_hintExists -->|yes| G_setHintText[Set hint text with warning and localhost requirement]
G_setHintText --> H_markHintError[Add error class to hint]
H_markHintError --> I_disableCopy[Disable copy command button]
F_hintExists -->|no| I_disableCopy
I_disableCopy --> J_return[Return]
B_checkUrl -->|true| K_clearInvalid[Disable invalid-url class on command section]
K_clearInvalid --> L_hintExists2{httpServerCommandHint != null}
L_hintExists2 -->|yes| M_clearHintError[Remove error class from hint]
L_hintExists2 -->|no| N_continue[Continue]
M_clearHintError --> N_continue
N_continue --> O_tryGetCommand[Try MCPServiceLocator.Server.TryGetLocalHttpServerCommand]
O_tryGetCommand --> P_updateField[Update command field or show server error]
P_updateField --> Q_end[End UpdateHttpServerCommandDisplay]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughCSS styling for error states is added to support visual error feedback. HTTP Local URL validation now checks if the URL is valid and displays error indicators (warning symbol, disabled field, error styling) when an invalid localhost URL is detected. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
Hi @dsarno! Another small UI correction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've left some high level feedback:
- Using the literal value "" in the command field may confuse users or be mistaken for an actual command; consider keeping the field empty/disabled and surfacing the error only via the hint/label and styling instead.
- The warning symbol ("⚠") added to the hint is a user-facing glyph; if you already have an icon system or standard warning style in the UI, consider reusing that instead of embedding a Unicode character in the text.
- The new CSS classes ("invalid-url" and "error") are quite generic; if they’re only used for this HTTP Local URL case, consider renaming them to something more specific (e.g., "http-local-invalid-url") to avoid style collisions and clarify intent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using the literal value "<Invalid Localhost URL>" in the command field may confuse users or be mistaken for an actual command; consider keeping the field empty/disabled and surfacing the error only via the hint/label and styling instead.
- The warning symbol ("⚠") added to the hint is a user-facing glyph; if you already have an icon system or standard warning style in the UI, consider reusing that instead of embedding a Unicode character in the text.
- The new CSS classes ("invalid-url" and "error") are quite generic; if they’re only used for this HTTP Local URL case, consider renaming them to something more specific (e.g., "http-local-invalid-url") to avoid style collisions and clarify intent.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Summary
Test plan
Before
After
Summary by Sourcery
Improve HTTP Local connection UI handling for invalid localhost URLs and maintain visibility of the command field.
Bug Fixes:
Enhancements:
Summary by CodeRabbit
Release Notes
Bug Fixes
Style
✏️ Tip: You can customize this high-level summary in your review settings.