Fix FlowControl condition logic and Playwright text locator handling by ShreehariMenon · Pull Request #266 · mozarkai/optics-framework

ShreehariMenon · 2026-03-11T09:31:59Z

Fixed FlowControl module condition evaluation to correctly return module execution results and handle else scenarios.

Improved Playwright locator normalization to support plain visible text selectors, enabling scroll_until_element_appears to work reliably.

All unit and feature tests are now passing successfully.

…ests passing

Copilot

Pull request overview

This PR aims to improve Optics’ conditional flow-control behavior and stabilize Playwright-based feature automation by adjusting locator handling and navigation/scroll timing.

Changes:

Updated FlowControl module-condition evaluation to treat module return values as the condition signal and execute targets/else consistently.
Modified Playwright driver behavior (timeouts, navigation wait condition, locator normalization, keypress waits, and scroll behavior).
Updated the YouTube Playwright feature test input, and added many api_details.har outputs under MagicMock/ (likely test artifacts).

Reviewed changes

Copilot reviewed 75 out of 75 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
tests/feature/engine/test_playwright_youtube.py	Updates the scroll target text used by the Playwright YouTube feature test.
optics_framework/engines/drivers/playwright.py	Adjusts Playwright session defaults, navigation behavior, locator normalization, keypress waits, and scroll implementation.
optics_framework/common/utils.py	Tightens CSV escape/unescape type contracts and refactors escape formatting.
optics_framework/api/flow_control.py	Changes module-condition evaluation logic and error handling behavior.
optics_framework/api/action_keyword.py	Makes scroll/swipe-until loops resilient to presence assertion failures and raises an OpticsError on timeout.
MagicMock/mock.config.execution_output_path/2975929025744/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2975928902160/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2975928091520/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2975926324688/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2975926277984/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2659841381424/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2659841031296/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2659840908448/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2659840601136/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2659840314832/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2625097222000/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2625097181104/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2625096938800/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2625095638368/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2625053294512/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2455961255184/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2455961061664/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2455960907664/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2455960684592/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2455959090144/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2354829118736/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2354828213952/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2354828209440/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2354828206944/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2354826435264/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2303835629152/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2303833457920/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2303833228400/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2303833045760/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2303832820960/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2219151890752/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2219151884416/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2219151714272/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2219151707168/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2219151291168/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2094717014848/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2094715670544/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2094715541392/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2094715509056/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2094715013936/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2082075543984/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2082075491936/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2082075477104/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2082075327856/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/2082056878032/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/1915648773488/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/1915648771520/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/1915648601616/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/1915647404672/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/1915647105248/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/1861717912000/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/1861717798656/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/1861715406880/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/1861715380464/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/1861715309968/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/1773426816592/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/1773426646992/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/1773426500928/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/1773408156512/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/1773365254368/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/1744383342784/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/1744383222288/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/1744381720384/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/1744381571824/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/1744378034896/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/1505733961792/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/1505733830768/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/1505733662464/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/1505715844016/api_details.har	Generated HAR output artifact (should not be committed).
MagicMock/mock.config.execution_output_path/1505715551264/api_details.har	Generated HAR output artifact (should not be committed).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-11T09:38:10Z

optics_framework/engines/drivers/playwright.py

    def scroll(self, direction: str = "down", pixels: int = 120, event_name=None):
-        for _ in range(2):
-            run_async(self.page.mouse.wheel(0, pixels if direction == "down" else -pixels))
+        for _ in range(20):
+            run_async(self.page.mouse.wheel(0, (pixels * 3) if direction == "down" else -(pixels * 3)))
            run_async(self.page.wait_for_timeout(120))


scroll() now performs 20 wheel events and triples the pixel delta each time, which can overscroll and makes driver.scroll(direction, pixels) behave very differently than requested (and more expensive). Consider making a single wheel call for the requested pixels (or keep the loop but match the requested magnitude) and let higher-level retry logic handle repetition.

Copilot · 2026-03-11T09:38:11Z

optics_framework/engines/drivers/playwright.py

            self.page = await self._context.new_page()
+            self.page.set_default_navigation_timeout(60000)
+            self.page.set_default_timeout(60000)


Default navigation/action timeouts are hard-coded to 60s here. Since the driver already supports timeout config (navigation_timeout_ms, etc.), consider pulling these from config (or setting them on the browser context) so timeout behavior is consistent and user-tunable.

Copilot · 2026-03-11T09:38:11Z

optics_framework/engines/drivers/playwright.py

        timeout_ms = int(self.config.get("navigation_timeout_ms", 60000))
        wait_until = self.config.get("navigation_wait_until", "domcontentloaded")

        try:
-            await self.page.goto(url, timeout=timeout_ms, wait_until=wait_until)
+            await self.page.goto(url, timeout=timeout_ms, wait_until="domcontentloaded")
        except PlaywrightTimeoutError as e:


_navigate_to reads navigation_wait_until into wait_until but ignores it and always uses wait_until="domcontentloaded". This makes the config ineffective and can make the warning log inaccurate. Use the wait_until variable when calling page.goto() (or remove the config option if it’s intentionally fixed).

Copilot · 2026-03-11T09:38:11Z

optics_framework/engines/drivers/playwright.py

        # Check if it's an XPath selector
        element_type = utils.determine_element_type(element)
        if element_type == "XPath":
            # If it doesn't already have the xpath= prefix, add it
            if not element.lower().startswith("xpath="):
                return f"xpath={element}"
-
+        # plain visible text support (important for YouTube test)
+        if isinstance(element, str) and not any(element.startswith(p) for p in ("css=", "xpath=", "text=", "//", "input", "#", ".")):
+            return f"text={element}"
        return element


The new plain-text normalization will incorrectly wrap many valid CSS selectors (e.g. div[aria-label=...]) as text=... because it doesn’t consult determine_element_type() and its prefix check is case-sensitive. This will break existing selector usage. Consider: (1) rely on determine_element_type(element) and only add text= when the type is Text, and (2) perform prefix checks case-insensitively.

Copilot · 2026-03-11T09:38:11Z

optics_framework/engines/drivers/playwright.py

            internal_logger.debug("[Playwright] Creating new tab for %s", app_name)
            self.page = await self._context.new_page()
+            # ⭐ very important stability fix



This comment adds an emoji and claims a “very important stability fix”, but no fix is implemented. Also, the new tab page doesn’t get the same default timeouts set in _launch_app_async. Suggest removing the emoji/comment and, if needed, actually apply the timeout/stability settings to the new page (or set them at the context level so all pages inherit).

Copilot · 2026-03-11T09:38:12Z

optics_framework/api/action_keyword.py

+            try:
+                result = self.verifier.assert_presence(
+                    element, timeout_str="3", rule="any")
+                if result:
+                    return
+            except Exception:
+                pass


These loops swallow all exceptions from verifier.assert_presence(), which can hide unexpected errors (not just “not present” failures). Since assert_presence raises AssertionError on a failed presence check, consider catching AssertionError specifically and letting other exception types surface.

Copilot · 2026-03-11T09:38:12Z

optics_framework/api/action_keyword.py

+            try:
+                result = self.verifier.assert_presence(
+                    element, timeout_str="3", rule="any")
+                if result:
+                    return
+            except Exception:
+                pass


These loops swallow all exceptions from verifier.assert_presence(), which can hide unexpected errors (not just “not present” failures). Since assert_presence raises AssertionError on a failed presence check, consider catching AssertionError specifically and letting other exception types surface.

Copilot · 2026-03-11T09:38:12Z

MagicMock/mock.config.execution_output_path/2975929025744/api_details.har

+{
+  "log": {
+    "version": "1.2",
+    "creator": {
+      "name": "Optics Framework",
+      "version": "1.0"


These MagicMock/mock.config.execution_output_path/**/api_details.har files look like generated execution artifacts from API logging, and committing many timestamped outputs will bloat the repo and create noisy diffs. Consider removing these from the PR and adding the execution output path pattern to .gitignore, or storing a single deterministic fixture under tests/fixtures/ if tests need it.

Copilot · 2026-03-11T09:38:12Z

optics_framework/engines/drivers/playwright.py

            browser = self.config.get("browser", "chromium")
-            headless = self.config.get("headless", False)
+            headless = True
            viewport = self.config.get("viewport", {"width": 1280, "height": 800})

            self._browser = await getattr(self._pw, browser).launch(headless=headless)


headless is now hard-coded to True, which overrides the driver configuration and prevents running Playwright in headed mode. Please restore reading this from config (e.g., defaulting to True for CI but allowing overrides) so existing consumers aren’t broken.

Copilot · 2026-03-11T09:38:13Z

optics_framework/engines/drivers/playwright.py

        run_async(self.page.keyboard.press(key))
+        # wait for youtube search results page load
+        try:
+            run_async(self.page.wait_for_load_state("networkidle", timeout=10000))
+        except Exception:
+            run_async(self.page.wait_for_timeout(4000))
+        # stabilize search navigation
+        run_async(self.page.wait_for_timeout(3000))


press_keycode() now adds YouTube-specific waits/timeouts on every key press, which can significantly slow down other flows and hide failures (broad except Exception). Consider scoping this behavior (e.g., only for Enter or behind a config flag) and catching only expected timeout exceptions.

… overscroll loops

sonarqubecloud · 2026-03-14T10:34:01Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Playwright driver stabilization + FlowControl condition fixes — all t…

f966d79

…ests passing

Copilot AI review requested due to automatic review settings March 11, 2026 09:32

Copilot started reviewing on behalf of ShreehariMenon March 11, 2026 09:32 View session

Copilot AI reviewed Mar 11, 2026

View reviewed changes

ShreehariMenon added 2 commits March 11, 2026 15:12

remove generated HAR artifacts and update gitignore

5bd4638

Refactor Playwright scroll behavior to respect pixel input and remove…

1c99063

… overscroll loops

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix FlowControl condition logic and Playwright text locator handling#266

Fix FlowControl condition logic and Playwright text locator handling#266
ShreehariMenon wants to merge 3 commits intomozarkai:mainfrom
ShreehariMenon:fix-csv-utils

ShreehariMenon commented Mar 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

sonarqubecloud bot commented Mar 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ShreehariMenon commented Mar 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Mar 14, 2026

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants