From 25bd7799771d4b843c220ce2e1b04b7bb7047b20 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 02:59:12 +0000 Subject: [PATCH] refactor(actions): extract _ref_result_coordinates for ref-lookup coordinate 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 Claude-Session: https://claude.ai/code/session_01YAf2ckWLpYZMsztaiAhreR --- src/frontend_visualqa/actions.py | 35 ++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/src/frontend_visualqa/actions.py b/src/frontend_visualqa/actions.py index 78c0427..bd146be 100644 --- a/src/frontend_visualqa/actions.py +++ b/src/frontend_visualqa/actions.py @@ -282,6 +282,26 @@ def _is_coordinate_pair(coordinates: Any) -> bool: return False +def _ref_result_coordinates(ref_result: dict[str, Any]) -> tuple[int, int] | None: + """Rounded viewport coordinates from a ``GET_ELEMENT_BY_REF_SCRIPT`` result. + + Returns ``None`` when the ref lookup did not succeed or the coordinates it + reported are missing/malformed, so callers can fall through to their own + fallback (raw coordinates, an error, or skipping an overlay preview). + Shared by :meth:`ActionExecutor._resolve_coordinates` and the + ``set_element_value`` paste-preview path in + :meth:`ActionExecutor._execute_expanded_tool`, which previously each + hand-rolled the identical success-check / pair-validation / round-to-int + sequence over the same script's result payload. + """ + if not ref_result.get("success"): + return None + coordinates = ref_result.get("coordinates") + if not _is_coordinate_pair(coordinates): + return None + return round(float(coordinates[0])), round(float(coordinates[1])) + + def render_action_trace( action_name: str, arguments: dict[str, Any], @@ -658,12 +678,12 @@ async def _execute_expanded_tool( if self._overlay is not None and ref: try: ref_info = await evaluate_tool_script(page, GET_ELEMENT_BY_REF_SCRIPT, ref) - coords = ref_info.get("coordinates") if ref_info.get("success") else None - if _is_coordinate_pair(coords): + coords = _ref_result_coordinates(ref_info) + if coords is not None: await self._best_effort_overlay_preview_action( action_type="set_element_value", - x=round(float(coords[0])), - y=round(float(coords[1])), + x=coords[0], + y=coords[1], ) except Exception: logger.debug("paste-effect preview failed for ref %s", ref, exc_info=True) @@ -725,10 +745,9 @@ async def _resolve_coordinates( result = await evaluate_tool_script(page, GET_ELEMENT_BY_REF_SCRIPT, ref) except Exception as exc: # pragma: no cover - defensive around browser evaluate failures result = {"success": False, "message": str(exc)} - if result.get("success"): - resolved_coordinates = result.get("coordinates") - if _is_coordinate_pair(resolved_coordinates): - return round(float(resolved_coordinates[0])), round(float(resolved_coordinates[1])) + resolved_coordinates = _ref_result_coordinates(result) + if resolved_coordinates is not None: + return resolved_coordinates if not has_coordinates: message = result.get("message", "Unknown error") raise BrowserActionError(f"{action_name} ref resolution failed for {ref}: {message}")