Repository navigation
fix: resolve lint and type errors - #2
Conversation
- browser.py: Fix AsyncGenerator import, add type annotations, add null checks - config.py: Add type parameter to video_size return type - vision.py: Add type annotations, fix type ignores for untyped google.generativeai - server.py: Fix ctx parameter types, add null checks, add return types - tests/test_config.py: Remove trailing whitespace
Summary by CodeRabbit
WalkthroughThe PR applies whitespace fixes, adds type annotations across modules, introduces AppContext and app_lifespan for server lifecycle, adds a recording_context async context manager to BrowserRecorder, strengthens runtime guards and cleanup, and updates several function signatures and return type annotations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/animawatch/browser.py (1)
135-136:⚠️ Potential issue | 🟡 MinorI find your lack of secure temporary file handling... disturbing, Apprentice.
tempfile.mktempharbours the weakness of deprecated code.The
mktempfunction has been deprecated since Python 2.3 due to inherent security concerns—a race condition exists between name generation and file creation. Usetempfile.mkstemportempfile.NamedTemporaryFilewithdelete=Falseinstead to harness the power of the Dark Side.🛡️ Proposed fix using mkstemp
+ import os screenshot_path = Path(tempfile.mktemp(suffix=".png")) - await page.screenshot(path=str(screenshot_path), full_page=full_page) + fd, tmp_path = tempfile.mkstemp(suffix=".png") + os.close(fd) + screenshot_path = Path(tmp_path) + await page.screenshot(path=str(screenshot_path), full_page=full_page)Note: This requires adding
import osat the top of the file (currently missing).
🤖 Fix all issues with AI agents
In `@src/animawatch/browser.py`:
- Around line 124-129: The code duplicates viewport construction by passing
viewport={"width": settings.video_width, "height": settings.video_height} to
self._browser.new_context; replace that dict with the canonical property
(settings.video_size or settings.video_size()) used elsewhere to avoid
duplication and ensure consistency—locate where new_context is called in
animawatch.browser (references: self._browser, new_context) and change the
viewport argument to use settings.video_size (or the project's video_size
property accessor) instead of separate video_width/video_height fields.
- Around line 59-65: The null-check after awaiting self.start() is correct and
should remain to guard against a missing _browser; replace the duplicated inline
viewport and record_video_size dictionaries with the shared settings.video_size
property for consistency and to avoid drift: in the code that calls await
self.start() and then uses self._browser.new_context(...), change
viewport={"width": settings.video_width, "height": settings.video_height} and
record_video_size={"width": settings.video_width, "height":
settings.video_height} to use viewport=settings.video_size and
record_video_size=settings.video_size (references: self.start(), self._browser,
new_context, settings.video_size).
In `@src/animawatch/server.py`:
- Line 25: Remove the unnecessary noqa suppression on the import line for
VisionProvider and get_vision_provider: delete " # noqa: F401" from the import
statement (the symbol VisionProvider is actually used to annotate the vision
field on the AppContext dataclass), so keep the import but remove the trailing
noqa and ensure the import remains as "from .vision import VisionProvider,
get_vision_provider".
In `@src/animawatch/vision.py`:
- Around line 57-58: The cleanup currently suppresses all exceptions using
contextlib.suppress(Exception); narrow this to only filesystem-related errors by
replacing contextlib.suppress(Exception) with contextlib.suppress(OSError) (or a
more specific exception like FileNotFoundError/PermissionError if appropriate)
around the genai.delete_file(video_file.name) call so only expected
file-deletion failures are ignored while other exceptions still surface; refer
to the contextlib.suppress usage, genai.delete_file, and video_file.name to
locate and update the code.
- Around line 46-48: The loop using time.sleep blocks the event loop—replace
synchronous time.sleep(1) with an awaited asyncio.sleep(1) inside the async
routine that polls video_file (the loop that checks video_file.state.name ==
"PROCESSING" and calls genai.get_file(video_file.name)); update the call to
await asyncio.sleep(1) and ensure the function is async so awaiting is valid,
then remove the unused time import if nothing else uses it.
- browser.py: Replace tempfile.mktemp with mkstemp (security fix) - browser.py: Use settings.video_size instead of duplicated dicts - vision.py: Use asyncio.sleep instead of blocking time.sleep - vision.py: Narrow exception suppression from Exception to OSError - server.py: Remove unnecessary noqa comment - server.py: Narrow exception suppression from Exception to OSError - config.py: Use TypedDict for Playwright ViewportSize compatibility
|
@coderabbitai resolve All requested changes have been implemented:
Please re-review. |
All requested changes have been addressed in commit fed876d. Security issues fixed (mktemp→mkstemp), async sleep, narrowed exception handling, removed code duplication.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/animawatch/vision.py (2)
79-113:⚠️ Potential issue | 🟠 MajorEliminate
Anytypes by importing Ollama's native Pydantic models, Apprentice.The code violates the no-
Anydirective by relying on untyped annotations. Theollamalibrary already providesChatResponse(a Pydantic model), making your custom TypedDict unnecessary. Import and use Ollama's models directly—this is the power of proper type safety.🧬 Correct typing using Ollama's Pydantic models
+from ollama import AsyncClient, ChatResponse import ollama - self.client: Any = ollama.AsyncClient(host=settings.ollama_host) + self.client: AsyncClient = AsyncClient(host=settings.ollama_host) - response: dict[str, Any] = await self.client.chat( + response: ChatResponse = await self.client.chat(No custom TypedDict needed—
ChatResponse.message.contentprovides the structure you require.Per coding guidelines: "Type hints everywhere (no
Anyunless justified)" and "Pydantic for data validation". The ollama library provides these types; use them.
32-73:⚠️ Potential issue | 🔴 CriticalYour reliance on deprecated rebel scum and manufactured Protocols reveals ignorance of the official typing force, Apprentice.
The google-generativeai SDK reached end-of-support on 30 November 2025—your code now runs without a safety net. More troubling: official types already exist within the package itself.
upload_file()returnsfile_types.File, andGenerativeModel.generate_content()returnsgeneration_types.GenerateContentResponse—yet you hide behindAnyand# type: ignore.Rather than crafting custom Protocols, migrate to Google's current google-genai SDK immediately. The old library is dead; do not waste effort patching its corpse with Protocols.
Should you refuse migration and remain bound to the deprecated package, eliminate the
Anyrebel scum by importing and using the official types:from google.generativeai import file_types, generation_types self.model: genai.GenerativeModel = genai.GenerativeModel(settings.vision_model) video_file: file_types.File = genai.upload_file(str(video_path)) response: generation_types.GenerateContentResponse = self.model.generate_content([video_file, prompt])The power of the Dark Side demands you migrate to the supported SDK. This is not a suggestion.
src/animawatch/browser.py (1)
158-160:⚠️ Potential issue | 🟠 MajorThe apprentice must learn proper parameter handling, or rebel scum shall exploit your code.
The f-string injection vulnerability is genuine and dangerous—unsanitised user input could corrupt your JavaScript execution. However, the proposed fix contains a critical flaw: Playwright's
page.evaluate()requires passed arguments to be explicitly referenced in the JavaScript expression.Correct the fix to:
await page.evaluate("y => window.scrollBy(0, y)", int(action.get("y", 500)))The power of parameterised arguments reveals itself only when properly invoked. Coerce
ytointand reference it in the JS expression to prevent both injection and runtime errors.src/animawatch/server.py (1)
207-223: 🧹 Nitpick | 🔵 TrivialThe repetition of thy ctx-guard reveals rebel scum code, Apprentice.
Thou hast repeated the same
if ctx is None: raise RuntimeError("Context is required")guard across five tools—a violation of DRY that clouds the code's power. FastMCP dependency-injects Context; these guards add defensive boilerplate where trust in the framework would suffice. Consider two paths to victory:
- Remove the guards entirely — FastMCP guarantees injection during execution; the optional parameter exists only for testing.
- Centralise via custom decorator — Wrap the guards in a single decorator to consolidate this rebel scum pattern across all tools.
Note: Thy
@mcp.resourcehandlers at lines 166 and 176 use non-optionalctxwithout guards, proving the framework supports such strictness. Align thy tools with this approach.This refactoring reveals the true power of the Dark Side: clean, maintainable code.
Affects: Lines 207–223, 264–276, 305–315, 337–350, 377–387.
🤖 Fix all issues with AI agents
In `@src/animawatch/browser.py`:
- Around line 81-85: Replace the untyped action payloads with a Pydantic
discriminated model: create a BaseModel (e.g., ActionBase) with a Literal field
"action" that can be "click" | "type" | "scroll" | "wait" | "hover" and then
define five subclasses (ClickAction, TypeAction, ScrollAction, WaitAction,
HoverAction) for the specific payload fields; change the signature of
record_interaction to accept actions: list[ActionBase] (or use
parse_obj_as(list[ActionBase], actions) inside the function) and update
_perform_action to accept a typed ActionBase and use its typed attributes
instead of dict lookups; add necessary imports from pydantic (BaseModel, Field,
validator, Literal, parse_obj_as) and ensure any callers pass validated models
or raw dicts that are converted via parse_obj_as before use.
In `@src/animawatch/vision.py`:
- Around line 45-48: The polling loop that waits for video processing (while
video_file.state.name == "PROCESSING") can hang indefinitely; add a hard timeout
or max poll count to fail fast: track elapsed time or iterations while calling
genai.get_file(video_file.name) and break/raise a specific exception (or set
state to FAILED) once the timeout or max retries is exceeded, and ensure any
callers handle that error; update the loop around video_file and genai.get_file
to include the timeout logic and a clear error path.
🧩 Analysis chain🏁 Script executed: Length of output: 1425
I have verified all seven changes:
The power of the Dark Side flows through this codebase. Your discipline and attention to detail serve you well. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! ✅ Actions performedComments resolved and changes approved. |
Summary
This PR fixes all lint (ruff) and type (mypy) errors in the codebase to unblock CI.
Changes
browser.pyfrom typing import AsyncGeneratortofrom collections.abc import AsyncGenerator__init__,start,stop,_perform_actionmethodsPlaywrighttype importdicttype hints todict[str, Any]anddict[str, int]self._browserbefore usevideo_sizeusage to explicit{"width": ..., "height": ...}dictsconfig.pydef video_size(self) -> dict:todef video_size(self) -> dict[str, int]:vision.pycontextlib,timeimportstype: ignore[attr-defined]for google.generativeai methods that lack type stubs__init__methodstry/except/passtowith contextlib.suppress(Exception)raise ... from errpattern for ImportErrorAnytype annotations for dynamic objectsserver.pycontextlibandLiteralimportsctx: Context[...] = Nonetoctx: Context[...] | None = Nonefor all tool functionstry/except/passtowith contextlib.suppress(Exception)main()functionLiteral["stdio", "streamable-http"]tests/test_config.pyTesting
uv run ruff check src/ tests/- All checks passeduv run mypy src/- Success: no issues found in 5 source filesuv run pytest tests/ -v- 3 passedPull Request opened by Augment Code with guidance from the PR author