-
Notifications
You must be signed in to change notification settings - Fork 578
Remove distribution settings scriptable object #473
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
Conversation
It was for safety as some were AI generated before. Only minor changes were made
Removes the McpDistributionSettings system that allowed different defaults for Asset Store vs git distributions. Hardcodes the default HTTP base URL to "http://localhost:8080" directly in HttpEndpointUtility and WebSocketTransportClient. Removes the setup window skip logic for remote defaults. It didn't work in practice, best thing to do is replace the placeholder in the UXML
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRemoves the unused MCP distribution settings ScriptableObject and inlined its behavior by hardcoding the localhost default HTTP base URL and always showing the setup window when needed, simplifying configuration logic and dependencies. Sequence diagram for the simplified setup window display logicsequenceDiagram
actor Developer
participant SetupWindowService
participant EditorPrefs
participant SetupWindow
Developer->>SetupWindowService: CheckSetupNeeded()
SetupWindowService->>EditorPrefs: GetBool(SETUP_COMPLETED_KEY)
EditorPrefs-->>SetupWindowService: setupCompleted
SetupWindowService->>EditorPrefs: GetBool(SETUP_DISMISSED_KEY)
EditorPrefs-->>SetupWindowService: setupDismissed
alt setup not completed or dismissed
SetupWindowService->>SetupWindow: Open()
SetupWindow-->>Developer: Display setup UI
else setup completed or dismissed
SetupWindowService-->>Developer: Do not show setup UI
end
Class diagram for HTTP endpoint utilities and WebSocket transport client after removing distribution settingsclassDiagram
class HttpEndpointUtility {
<<static>>
const string PrefKey
const string DefaultBaseUrl
}
class WebSocketTransportClient {
+BuildWebSocketUri(string baseUrl) Uri
}
HttpEndpointUtility <.. WebSocketTransportClient : uses DefaultBaseUrl
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@msanatan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 1 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (6)
✨ Finishing touches
🧪 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 |
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:
- The default HTTP base URL is now hardcoded as "http://localhost:8080" in multiple places; consider centralizing this in a single constant or helper to avoid duplication and reduce the chance of divergence.
- Removing the
McpDistribution.Settings.skipSetupWindowWhenRemoteDefaultlogic changes behavior for remote/hosted distributions by always showing the setup window; double-check that this behavior change is desired for all distributions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The default HTTP base URL is now hardcoded as "http://localhost:8080" in multiple places; consider centralizing this in a single constant or helper to avoid duplication and reduce the chance of divergence.
- Removing the `McpDistribution.Settings.skipSetupWindowWhenRemoteDefault` logic changes behavior for remote/hosted distributions by always showing the setup window; double-check that this behavior change is desired for all distributions.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
It wasn't used, and this should help make startup slightly faster
Summary by Sourcery
Remove unused MCP distribution configuration and default remote URL handling, falling back to a fixed local endpoint instead.
Enhancements:
Chores: