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

    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

            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

+        # 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:12Z

+{
+  "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:13Z

        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.

Signed-off-by: Shreehari Menon <168951991+ShreehariMenon@users.noreply.github.com>

… 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

thakur-patel · 2026-05-27T07:25:32Z

@ShreehariMenon could you resolve all the code comments by CoPilot AI before we can go ahead with a merge?
@malto101 a review from you would help here

Signed-off-by: Shreehari Menon <168951991+ShreehariMenon@users.noreply.github.com>

malto101 · 2026-05-27T15:01:06Z

        # Use mapped key or the keycode string directly (Playwright accepts key names)
        key = key_map.get(keycode, keycode)
        run_async(self.page.keyboard.press(key))
+        # wait for youtube search results page load


can we remove indication of YouTube in drivers

malto101 · 2026-05-27T15:03:00Z

-        for _ in range(2):
-            run_async(self.page.mouse.wheel(0, pixels if direction == "down" else -pixels))
-            run_async(self.page.wait_for_timeout(120))
+        scroll_multiplier = int(self.config.get("scroll_multiplier", 1))


can all the config fetch like navigation timeout and scroll multiplier be done on init, so new contributors can have an idea on existing configs for the driver

would be even better, if its also part of the docs

malto101 · 2026-05-27T15:05:53Z

-                element, timeout_str="3", rule="any")
-            if result:
-                break
+            try:


could you tell the reason for changing this ?

This change was added to prevent transient assert_presence() failures from terminating the retry loop immediately during scroll/swipe operations while the page content is still loading or updating.

Updated the implementation to catch only AssertionError so unexpected exceptions still surface normally.

@thakur-patel I am a bit confused on this one,
could you share your inputs?

malto101 · 2026-05-27T15:06:39Z

                var_name = var_name[2:-1].strip()
            else:
-                internal_logger.warning(
+                internal_logger.exception(


this is fine as a warning,
as its not breaking the framework

malto101 · 2026-05-27T15:19:56Z

        actual_cond = cond_str[1:].strip() if cond_str.startswith("!") else cond_str
        return self.modules.get_module_definition(actual_cond) is not None

    def _handle_module_condition(self, cond_str: str, target: str) -> Optional[List[Any]]:


This change will mostly need to be removed as it changes the existing semantics.

There are 3 parts involved here:

Inverted flag (!)

Condition statement/module

Target statement/module

The current behavior is:

If not inverted:

if condition_true: execute target else: do nothing

If inverted:

if condition_true: do nothing else: execute target

In the new implementation, after the inversion logic is applied, we effectively end up with:

if condition_true: return result if result else execute target else: execute target

This means the target module is executed when the condition evaluates to false, which is not the current behavior for non-inverted conditions.

Additionally, when condition_true is true and the condition module returns a non-empty result, the target module is not executed at all and the condition result is returned instead.

So this is not a refactor of the existing logic; it changes the behavior of module conditions significantly.

malto101 · 2026-05-27T15:20:19Z

rest all looks good

sonarqubecloud · 2026-05-27T16:12:37Z

Quality Gate passed

Issues
1 New issue
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 and others added 3 commits March 11, 2026 15:11

Add MagicMock and HAR files to .gitignore

4a3ae25

Signed-off-by: Shreehari Menon <168951991+ShreehariMenon@users.noreply.github.com>

remove generated HAR artifacts and update gitignore

5bd4638

Refactor Playwright scroll behavior to respect pixel input and remove…

1c99063

… overscroll loops

ShreehariMenon and others added 5 commits May 27, 2026 13:14

Retrigger pre-commit checks

5b74a88

Merge remote-tracking branch 'origin/main' into fix-csv-utils

f818b48

Merge branch 'main' into fix-csv-utils

b152142

Signed-off-by: Shreehari Menon <168951991+ShreehariMenon@users.noreply.github.com>

Address Copilot review comments

000f2ec

Fix selector normalization logic

bdb9890

malto101 reviewed May 27, 2026

View reviewed changes

ShreehariMenon added 2 commits May 27, 2026 21:35

Revert FlowControl semantic behavior changes

dbd6c41

Refine Playwright driver behavior based on review feedback

ebf28e1

Uh oh!

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!

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

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!

thakur-patel commented May 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

malto101 commented May 27, 2026

Uh oh!

sonarqubecloud Bot commented May 27, 2026

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants