refactor(browser): type BrowserManager.__aexit__ with stdlib exception types - #291
Conversation
…n types BrowserManager.__aexit__ typed exc_type as bare `type | None` and traceback as `Any`, even though Python's own contextmanager protocol has concrete stdlib types for both: `type[BaseException] | None` for the exception class and `types.TracebackType | None` for the traceback. This is the same Any->concrete-type vein this repo has mined repeatedly (most recently #277-#290), just applied to a dunder signature using a stdlib type instead of a project type. Annotation-only, zero behavior change: the method body only calls self.close() and never inspects any of its three parameters. Verified BrowserManager is only ever driven through `async with` (13 call sites in tests/test_browser.py) with no direct __aexit__ invocation anywhere in the repo, so no caller depends on the old signature. Co-authored-by: Claude <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
ChangesBrowserManager exit typing
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This PR only narrows the type annotations for BrowserManager.aexit without changing runtime behavior; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
What
BrowserManager.__aexit__insrc/frontend_visualqa/browser.pywas typed:exc_typeused a baretype | NoneandtracebackusedAny, even though Python's own async-context-manager protocol has concrete stdlib types for both:type[BaseException] | Nonefor the exception class andtypes.TracebackType | Nonefor the traceback object. This repo has repeatedly tightenedAny-typed parameters to concrete types where every real call site's shape is already known (most recently #277/#281/#283/#284/#285/#288/#289/#290) — this is the same pattern applied to a dunder signature, using a stdlib type rather than a project type since none of the three parameters are project-defined.Why it's safe
await self.close(), and never inspectsexc_type,exc, ortraceback.BrowserManageris only ever driven throughasync with BrowserManager(...) as manager:(13 call sites acrosstests/test_browser.py); nothing in the repo calls__aexit__directly with explicit arguments, so no caller depends on the old bare signature.contextlib/typeshed's standard__aexit__signature shape.Verification
uv run pytest tests/→ 322 passed / 21 pre-existing environment-only Chromium-sandbox failures (identical before/after, confirmed viagit stash).uv run ruff check src/ tests/→ clean.uv run ruff format --check src/ tests/→ same 7 pre-existing "would reformat" files as onmain(notbrowser.py); no new formatting drift introduced.1 file changed, 3 lines net (+7/-1).
Generated by Claude Code
Note
Low Risk
Annotation-only signature change with no runtime or control-flow impact.
Overview
Typing-only update to
BrowserManager.__aexit__:exc_typeis nowtype[BaseException] | NoneandtracebackisTracebackType | Noneinstead of a baretypeandAny.The method still only
awaitsclose()and does not inspect those arguments. No behavior change.Reviewed by Cursor Bugbot for commit 988a8a9. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit