Conversation
|
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 (3)
WalkthroughConfiguration file removed; new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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🧪 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
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/framex/plugins/proxy/__init__.py (1)
45-60:⚠️ Potential issue | 🟡 MinorBug: Logger message uses stale loop variable.
After the
for url in settings.proxy_url_listloop completes,urlholds only the last URL. If multiple proxy URLs are configured, the success message will incorrectly report only the final URL.🐛 Proposed fix
- for url in settings.proxy_url_list: - await self._parse_openai_docs(url) + for proxy_url in settings.proxy_url_list: + await self._parse_openai_docs(proxy_url) + logger.success(f"Succeeded to parse openai docs from {proxy_url}") if settings.proxy_functions: for url, funcs in settings.proxy_functions.items(): for func in funcs: await self._parse_proxy_function(func, url) else: # pragma: no cover logger.debug("No proxy functions to register") - - logger.success(f"Succeeded to parse openai docs form {url}")Note: Also fixes the typo "form" → "from".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framex/plugins/proxy/__init__.py` around lines 45 - 60, In on_start, the logger.success call uses the loop variable url which is stale after iterating proxy_url_list; change the success log to reference the full list or a derived value (e.g., join settings.proxy_url_list or use first/all URLs) instead of url and fix the typo "form"→"from"; update the logger.success in the on_start method of the proxy plugin to produce an accurate message (e.g., include ", parsed from {', '.join(settings.proxy_url_list)}") so it doesn't rely on the loop variable.
🧹 Nitpick comments (4)
src/framex/driver/ingress.py (1)
29-36: Consider moving imports to module level.The
osimport andsettingsimport inside the function body add overhead on each call. If there are no circular import issues, consider moving them to the top of the file where other imports reside.♻️ Proposed refactor
Move the import to module level:
+import os + from framex.adapter import get_adapter from framex.consts import BACKEND_NAME from framex.driver.application import create_fastapi_application from framex.driver.auth import api_key_header, auth_jwt from framex.driver.decorator import api_ingress from framex.log import setup_logger from framex.plugin.model import ApiType, PluginApi from framex.utils import escape_tag, shorten_str +from framex.config import settingsThen simplify the endpoint:
`@app.get`("/version") async def version() -> str: - import os - - from framex.config import settings - return settings.server.reversion or os.getenv("REVERSION") or "unknown"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framex/driver/ingress.py` around lines 29 - 36, The version endpoint currently imports os and framex.config.settings inside the version() function which adds per-request overhead; move "import os" and "from framex.config import settings" to the module-level imports and then simplify the version() handler (function name: version) to return settings.server.reversion or os.getenv("REVERSION") or "unknown" directly without inline imports; ensure there are no circular-import issues after relocating the imports.src/framex/plugins/proxy/config.py (2)
44-53: Consider extracting the matching logic to reduce duplication.The pattern matching logic here is similar to
AuthConfig._is_url_protectedinsrc/framex/config.py(lines 86-117). Both handle exact matches and prefix wildcards with/*. Consider extracting a shared utility function if this pattern appears elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framex/plugins/proxy/config.py` around lines 44 - 53, The duplicated path-matching logic in ProxyConfig._match_rules and AuthConfig._is_url_protected should be extracted into a shared utility (e.g., a function like match_pattern(pattern: str, path: str) or match_rules(whitelist: list[str], path: str) in a common utils/module) and both locations should call that utility; update ProxyConfig._match_rules to delegate to the new helper and refactor AuthConfig._is_url_protected to use the same helper so exact matches, the "/*" global match, and prefix wildcard matches (rule.endswith("/*") and path.startswith(rule[:-1])) behave identically. Ensure the helper is unit-tested or used by existing tests and exported where needed.
55-61: Minor: Consider a custom exception class for clearer error handling.The static analysis tool flags the inline error message (TRY003). While this is a low priority, using a dedicated exception or a shorter message would address it:
- raise ValueError("proxy_urls must be a list or a dict") # pragma: no cover + raise TypeError("Invalid proxy_urls type") # pragma: no cover🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framex/plugins/proxy/config.py` around lines 55 - 61, The property proxy_url_list raises a generic ValueError for invalid proxy_urls; define a custom exception (e.g., class ProxyConfigError(Exception)) in this module and replace the ValueError in proxy_url_list with raising ProxyConfigError using a concise message like "proxy_urls must be list or dict" to satisfy the static analyzer and provide clearer error semantics.tests/api/test_base.py (1)
10-13: Consider asserting the response body format.The test only verifies the status code but doesn't validate that the response is a non-empty string. Since the endpoint can return
settings.server.reversion,REVERSIONenv var, or"unknown", consider asserting the response body is at least a valid string:def test_get_version(client: TestClient): """Test the version endpoint.""" r = client.get("/version") assert r.status_code == 200 + assert isinstance(r.json(), str)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/api/test_base.py` around lines 10 - 13, The test test_get_version only asserts status code; extend it to also validate the response body by asserting that the returned value from client.get("/version") is a non-empty string (e.g., verify isinstance(r.text, str) and r.text.strip() != ""), so you catch cases where the endpoint returns empty or unexpected content; update the assertions in test_get_version to check both r.status_code == 200 and the non-empty string condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/framex/plugins/proxy/__init__.py`:
- Around line 45-60: In on_start, the logger.success call uses the loop variable
url which is stale after iterating proxy_url_list; change the success log to
reference the full list or a derived value (e.g., join settings.proxy_url_list
or use first/all URLs) instead of url and fix the typo "form"→"from"; update the
logger.success in the on_start method of the proxy plugin to produce an accurate
message (e.g., include ", parsed from {', '.join(settings.proxy_url_list)}") so
it doesn't rely on the loop variable.
---
Nitpick comments:
In `@src/framex/driver/ingress.py`:
- Around line 29-36: The version endpoint currently imports os and
framex.config.settings inside the version() function which adds per-request
overhead; move "import os" and "from framex.config import settings" to the
module-level imports and then simplify the version() handler (function name:
version) to return settings.server.reversion or os.getenv("REVERSION") or
"unknown" directly without inline imports; ensure there are no circular-import
issues after relocating the imports.
In `@src/framex/plugins/proxy/config.py`:
- Around line 44-53: The duplicated path-matching logic in
ProxyConfig._match_rules and AuthConfig._is_url_protected should be extracted
into a shared utility (e.g., a function like match_pattern(pattern: str, path:
str) or match_rules(whitelist: list[str], path: str) in a common utils/module)
and both locations should call that utility; update ProxyConfig._match_rules to
delegate to the new helper and refactor AuthConfig._is_url_protected to use the
same helper so exact matches, the "/*" global match, and prefix wildcard matches
(rule.endswith("/*") and path.startswith(rule[:-1])) behave identically. Ensure
the helper is unit-tested or used by existing tests and exported where needed.
- Around line 55-61: The property proxy_url_list raises a generic ValueError for
invalid proxy_urls; define a custom exception (e.g., class
ProxyConfigError(Exception)) in this module and replace the ValueError in
proxy_url_list with raising ProxyConfigError using a concise message like
"proxy_urls must be list or dict" to satisfy the static analyzer and provide
clearer error semantics.
In `@tests/api/test_base.py`:
- Around line 10-13: The test test_get_version only asserts status code; extend
it to also validate the response body by asserting that the returned value from
client.get("/version") is a non-empty string (e.g., verify isinstance(r.text,
str) and r.text.strip() != ""), so you catch cases where the endpoint returns
empty or unexpected content; update the assertions in test_get_version to check
both r.status_code == 200 and the non-empty string condition.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
config.jssrc/framex/driver/application.pysrc/framex/driver/ingress.pysrc/framex/plugins/proxy/__init__.pysrc/framex/plugins/proxy/config.pytests/api/test_base.py
💤 Files with no reviewable changes (1)
- config.js
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests