Conversation
The class name `Enabled` was confusing because it described a state rather than the concept being controlled. `Visibility` better expresses what the transform does: control which components are visible to clients. - Class: Enabled → Visibility - File: enabled.py → visibility.py - Context method: reset_components() → reset_visibility() - Internal metadata key: "enabled" → "visibility"
|
Warning Rate limit exceeded
⌛ 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 (1)
WalkthroughThis pull request performs a comprehensive terminology refactor across the codebase, renaming the "Enabled" system to "Visibility" throughout. The changes rename the Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/development/v3-notes/v3-features.mdx (1)
296-303: Inconsistent method reference:reset_components()should bereset_visibility().Line 299 still references
reset_components()in the method list, but the example on line 289 correctly usesreset_visibility(). This inconsistency should be fixed for documentation accuracy.📝 Proposed fix
Session visibility methods: - `await ctx.enable_components(...)`: Enable components for this session - `await ctx.disable_components(...)`: Disable components for this session -- `await ctx.reset_components()`: Clear session rules, return to global defaults +- `await ctx.reset_visibility()`: Clear session rules, return to global defaults
🧹 Nitpick comments (3)
docs/python-sdk/fastmcp-server-server.mdx (1)
1-4: Adddescriptionfield to frontmatter.As per coding guidelines, MDX documentation pages must include both
titleanddescriptionin the YAML frontmatter.Suggested fix
--- title: server sidebarTitle: server +description: API reference for the FastMCP server module, including server configuration, tools, resources, prompts, and transforms. ---src/fastmcp/server/transforms/__init__.py (1)
223-223: Consider removing the unusednoqadirective.Static analysis indicates the
# noqa: E402comment may be unnecessary. The E402 rule (module-level import not at top of file) might not be triggering here. Consider removing it if it's no longer needed.♻️ Suggested fix
-from fastmcp.server.transforms.visibility import Visibility, is_enabled # noqa: E402 +from fastmcp.server.transforms.visibility import Visibility, is_enabledsrc/fastmcp/server/transforms/visibility.py (1)
92-109: Align__repr__output with boolean constructor values.
__repr__currently emitsVisibility(enable, ...)for most cases butVisibility(True, match_all=True)formatch_all. Using booleans everywhere better mirrors the constructor and examples.♻️ Proposed update for consistent repr output
- action = "enable" if self._enabled else "disable" + enabled = self._enabled if self.match_all: - return f"Visibility({self._enabled}, match_all=True)" + return f"Visibility({enabled}, match_all=True)" parts = [] if self.names: parts.append(f"names={set(self.names)}") @@ if self.tags: parts.append(f"tags={set(self.tags)}") if parts: - return f"Visibility({action}, {', '.join(parts)})" - return f"Visibility({action})" + return f"Visibility({enabled}, {', '.join(parts)})" + return f"Visibility({enabled})"
| Adds a visibility transform that marks matching components as disabled. | ||
| Components can be re-enabled by calling enable() with matching criteria | ||
| (the later transform wins). |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "base.py" -type f | grep -E "(fastmcp|server.*provider)" | head -20Repository: jlowin/fastmcp
Length of output: 263
🏁 Script executed:
git ls-files | grep -E "fastmcp.*base\.py|server.*providers.*base\.py"Repository: jlowin/fastmcp
Length of output: 346
🏁 Script executed:
fd "base.py" --type fRepository: jlowin/fastmcp
Length of output: 346
🏁 Script executed:
cat -n src/fastmcp/server/providers/base.py | head -350Repository: jlowin/fastmcp
Length of output: 15488
🏁 Script executed:
wc -l src/fastmcp/server/providers/base.pyRepository: jlowin/fastmcp
Length of output: 98
🏁 Script executed:
rg -n -B2 -A5 'def enable\(' src/fastmcp/server/providers/base.pyRepository: jlowin/fastmcp
Length of output: 332
🏁 Script executed:
rg -n -B2 -A5 'def disable\(' src/fastmcp/server/providers/base.pyRepository: jlowin/fastmcp
Length of output: 273
🏁 Script executed:
sed -n '492,538p' src/fastmcp/server/providers/base.pyRepository: jlowin/fastmcp
Length of output: 1897
🏁 Script executed:
sed -n '541,578p' src/fastmcp/server/providers/base.pyRepository: jlowin/fastmcp
Length of output: 1471
🏁 Script executed:
cat -n docs/python-sdk/fastmcp-server-providers-base.mdx | sed -n '260,300p'Repository: jlowin/fastmcp
Length of output: 1891
Update enable() documentation to match disable() and source code
The disable() change is correct, but the documentation for enable() at line 264 needs updating. The source code docstring for enable() says "Adds a visibility transform" (matching disable()), but the documentation still shows "Adds an enabled transform". Change line 264 from "Adds an enabled transform" to "Adds a visibility transform" for consistency with both the source and the disable() method.
The
Enabledclass name was awkward—it describes a state rather than the concept being controlled. Renaming toVisibilitymakes the API clearer since the transform controls which components are visible to clients.The
enable()anddisable()methods on servers/contexts remain unchanged—those are action verbs that still make sense.