refactor(actions): extract _ref_result_coordinates for ref-lookup coordinate resolution - #293
Conversation
…rdinate resolution Both call sites that resolve a Navigator element ref to viewport coordinates via GET_ELEMENT_BY_REF_SCRIPT hand-rolled the identical success-check / coordinate-pair-validation / round-to-int sequence over the same script's result payload: - ActionExecutor._resolve_coordinates (the click/hover/scroll targeting path) - ActionExecutor._execute_expanded_tool's set_element_value paste-preview Extracted the shared shape into a module-level `_ref_result_coordinates` helper that returns `tuple[int, int] | None`, so both sites just check for `None` and fall through to their own fallback (raw coordinates, an error, or skipping the overlay preview). Behavior-preserving: the helper returns `None` in exactly the two cases the inline code fell through on (lookup not successful, or coordinates missing/malformed), and applies the same `round(float(...))` conversion. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAf2ckWLpYZMsztaiAhreR
|
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. 📝 WalkthroughWalkthroughThe change adds a shared helper for validating and rounding coordinates from successful reference lookups. The overlay preview and ref-based action resolution now use this helper. ChangesCoordinate validation
Estimated code review effort: 2 (Simple) | ~5 minutes Merge Risk: ⚪ Minimal · up to This change centralizes existing coordinate decoding without an indicated behavior or API change. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
The structural issue
actions.pyhas exactly two places that turn a Navigator elementrefinto concrete viewport coordinates by running the SDK'sGET_ELEMENT_BY_REF_SCRIPT, and each hand-rolled the identical decode sequence over that script's result payload — checksuccess, pullcoordinates, validate the shape with_is_coordinate_pair, thenround(float(...))both components:ActionExecutor._resolve_coordinates— the click / hover / mouse_down / mouse_up / scroll targeting path:ActionExecutor._execute_expanded_tool,set_element_valuepaste-preview branch:Same script, same payload contract, same three-step decode, expressed two different ways ~70 lines apart.
Why the refactor is an improvement
Extracts a module-level
_ref_result_coordinates(ref_result) -> tuple[int, int] | Nonenext to the existing_is_coordinate_pairhelper it builds on. Both call sites now just ask for the coordinates and check forNone, then fall through to their own (genuinely different) fallback: rawcoordinates/ aBrowserActionErrorfor the targeting path, silently skipping the overlay preview for the paste path.That puts the "what a successful ref lookup looks like, and how its floats become viewport ints" rule in one place. Today the two spellings agree only by coincidence — the
successgate, the pair validation, and the rounding are each an independent chance to drift (e.g. one site gaining a truncate-vs-round or a missing-successtolerance the other doesn't). It also makes the payload-shaped decode independently readable and named, rather than inlined inside a 60-line dispatch branch.Why it is safe
Nonein exactly the two cases the inline code fell through on —successfalsy, orcoordinatesfailing_is_coordinate_pair— and applies the sameround(float(...))conversion on the happy path. In_resolve_coordinatesthe "success but malformed coordinates" case still falls through to the ref-resolution-failed / coordinate-fallback branch, as before. Neither call site's surroundingtry/exceptscope changed, so an evaluate failure or a malformed payload is still caught where it was.uv run pytest tests/→ 322 passed / 21 failed, byte-identical to the pre-change baseline on this branch point (the 21 are the repo's pre-existing environment-only Chromium-sandbox failures intest_browser.py/test_live_runner.py).tests/test_actions.pyalone: 40/40.uv run ruff checkandruff format --checkclean on the touched file;python -m py_compileclean.dict[str, Any] -> tuple[int, int] | None) matches what both call sites already do with the value.Generated by Claude Code
Note
Low Risk
Internal refactor of coordinate decoding with no intended behavior or API change.
Overview
Deduplicates how
GET_ELEMENT_BY_REF_SCRIPTresults become viewport ints.Adds
_ref_result_coordinates, which checkssuccess, validates the pair with_is_coordinate_pair, and rounds to ints (or returnsNone)._resolve_coordinatesand theset_element_valueoverlay preview now share that helper instead of inlining the same decode. Behavior and fallbacks are unchanged.Reviewed by Cursor Bugbot for commit 25bd779. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit