Conversation
WalkthroughAdds runtime configuration-aware proxy routing and coroutine validation to the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant on_proxy_wrapper
participant Settings as "settings.proxy_functions"
participant ProxyService
participant SafeCallable
Caller->>on_proxy_wrapper: invoke decorated function (async)
on_proxy_wrapper->>Settings: check fully-qualified name in proxy_functions?
alt configured for proxy
on_proxy_wrapper->>ProxyService: route call via proxy logic
ProxyService-->>on_proxy_wrapper: result / error
alt proxy error
on_proxy_wrapper->>SafeCallable: invoke original implementation
SafeCallable-->>on_proxy_wrapper: result
end
else not configured
on_proxy_wrapper->>SafeCallable: invoke original implementation
SafeCallable-->>on_proxy_wrapper: result
end
on_proxy_wrapper-->>Caller: return result or raise
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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! |
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)
tests/test_plugins.py (1)
256-261:⚠️ Potential issue | 🟡 MinorTypo in function name:
supply_execption→supply_exception.Minor typo — "execption" should be "exception". Since this is only test-internal, it's non-blocking but worth fixing for readability.
🧹 Nitpick comments (3)
tests/test_plugins.py (1)
237-237: Preferis Noneover== None.Using
is Noneis the idiomatic Python comparison forNoneand avoids potential issues with objects that override__eq__.Proposed fix
- assert resolve_default(Optional[int]) == None # noqa + assert resolve_default(Optional[int]) is Nonesrc/framex/plugin/on.py (2)
122-139: Broadexcept Exceptionmay mask legitimate application errors in the fallback path.Catching all exceptions means transient proxy-layer issues and permanent application bugs (e.g.,
TypeError,ValueErrorfrom bad input) are handled identically. When the original call fails due to a real bug, the fallback will almost certainly re-raise the same error, but only after emitting a misleading "Falling back" warning.Consider narrowing the catch to proxy/network-specific exceptions, or at least re-raising exceptions that clearly originate from the user's code rather than the proxy/decorator machinery.
Additionally, the warning on Line 125-128 is logged before the fallback is attempted, so on failure of both paths the log trail is confusing.
♻️ Suggested approach
- except Exception as e: + except (ConnectionError, TimeoutError, RuntimeError) as e: from framex import logger logger.warning( f"[Proxy Fallback] Async wrapper for '{full_func_name}' failed with {type(e).__name__}: {e}. " - f"Falling back to the original implementation." + f"Attempting fallback to the original implementation." )Adjust the exception tuple to match the actual proxy-layer errors you expect. If broad catching is intentional, add a comment explaining why.
148-152:proxy_func_setis rebuilt on every invocation.
set(chain.from_iterable(proxy_settings.proxy_functions.values()))allocates a new set each time the wrapped function is called. If the proxy config is static (or changes infrequently), consider caching it — e.g., building the set once and invalidating on config reload.If runtime mutability of
proxy_functionsis a hard requirement, this is fine as-is for correctness, but worth keeping in mind for hot paths.
|
@coderabbitai cache "proxy_functions" |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 10390
Key findings:
✏️ Learnings added
|
Summary by CodeRabbit
New Features
Bug Fixes
Tests