Skip to content
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

Fix ESLint rules-of-hooks warnings #218

Merged

Conversation

cliffhall
Copy link
Contributor

@cliffhall cliffhall commented Mar 27, 2025

This fixes #217 about the linter's warning that Hooks must be run in order each time.

  • In App.tsx
    • move the conditional that returns suspense if the path is oauth/callback to the end of the component after all hooks, rendering either suspense or the normal component.
    • move handleApproveSampling and handleRejectSampling constants down below all the hooks for clarity. There are a lot of hooks so finding the end of them is a scroll, and these function constants aren't referenced until the rendering section below anyway.
    • In the useEffect used to auto-connect if serverUrl is provided in URL params, add the connectMcpServer method as a dependency, quieting the ESLint complaint about it.
    • Prettier fixed indenting at the end, so it looks like a bigger change than it actually is.

Motivation and Context

Software correctness.

How Has This Been Tested?

  1. cd to inspector/client
  2. run eslint

Previous output

Screenshot 2025-03-27 at 4 34 54 PM

After fix (other warnings still exist, but are out of scope for this fix)

Screenshot 2025-03-27 at 4 36 21 PM
  • All normal Inspector functionality tested with normal SSE and STDIO servers as usual.
  • The conditional path that leads to suspense was when the path was oauth/callback, tested with this minimal GitHub OAuth MCP server.
oauth

Breaking Changes

Nope.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

Fixes #217 which was raised in response to this PR which was going to incidentally suppress rules-of-hooks warnings. I wanted to fix the problem instead.

…time.

* In App.tsx
  - move the conditional that returns suspense if the path is oauth/callback to the end of the component after all hooks, rendering either suspense or the normal component.
  - move handleApproveSampling and handleRejectSampling functions down below all the hooks for clarity. There are a lot of hooks so finding the end of them is a scroll, and these function constants aren't referenced until the rendering section below anyway.
@cliffhall cliffhall requested review from olaservo and tadasant March 27, 2025 20:07
olaservo
olaservo previously approved these changes Mar 28, 2025
Copy link
Collaborator

@olaservo olaservo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for testing that the Oauth callback is still working! 🥇

…ing and hopefully make the PR diff easier to approve.
@cliffhall cliffhall requested a review from olaservo March 28, 2025 20:24
@olaservo olaservo merged commit 75537c7 into modelcontextprotocol:main Mar 29, 2025
2 checks passed
@cliffhall cliffhall deleted the fix-rules-of-hooks-warning branch March 29, 2025 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix ESlint rules-of-hooks warnings in App.tsx
3 participants