Skip to content

fix(selectors): anchor the New project CTA on add, and revert a guard I got wrong #740

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ffroliva merged 1 commit into from
Sep 7, 2026
Merged

fix(selectors): anchor the New project CTA on add, and revert a guard I got wrong #740

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,28 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Fixed

- **The "+ New project" CTA is found by structure again, not by English.** Its Tier-1 anchors
all targeted the `add_2` ligature. The migrated `flow.google.com` gallery renders **`add`**
under a `<mat-icon>` and renders `add_2` nowhere on that surface, so every structural entry
missed and the CTA was reached only by `button:has-text('New project')` — the localised-text
anti-pattern Tier 1 exists to avoid, and one that fails outright on a non-English migrated
profile. Tier 1 now leads with a class-only `add` anchor covering both carriers; measured on
the live gallery (control: 47 ligature nodes) it matches 1 where every previous entry
matched 0.

Also removed: `button:text-matches('^\+\s+\S+$', 'i')`. It is not a valid Playwright
selector and **raised on every evaluation** — observed twice against the live gallery — with
`except Exception: continue` swallowing it, so it had never matched anything on any host
while costing a round trip per attempt.

**This reverts the guard added moments earlier in the same release.** That guard raised
`FlowHostMigratedError` claiming the migrated host "renders no '+ New project' control gflow
can drive". It was an unproven negative, generalised from a sweep of two other surfaces to a
third that was never probed, and a live run disproved it in one click by creating a project
there. On a non-EN migrated profile it would have told the user that host has no such
control — false, and a dead end — when the real fix was to anchor on `add`.
([#730](https://github.com/ffroliva/gflow-cli/issues/730))

- **A migrated-host gallery no longer reports a missing control as selector drift.**
`NEW_PROJECT_SELECTORS` anchors on the `add_2` ligature. The migrated `flow.google.com`
frontend renders **`add`** and renders `add_2` **nowhere** — composer `add=1/add_2=0`,
Expand Down
46 changes: 22 additions & 24 deletions src/gflow_cli/api/transports/ui_automation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@
ConfigurationError,
ContentPolicyError,
FlowAppError,
FlowHostMigratedError,
GFlowError,
RateLimitError,
UiSelectorDriftError,
Expand Down Expand Up @@ -357,10 +356,22 @@ async def _capture_debug_screenshot(
# `_enter_editor` now names the host rather than blaming this cascade when it is reached.
NEW_PROJECT_SELECTORS = (
# Tier 1 — structural / icon: locale-invariant.
"button:has(i.google-symbols:text('add_2'))",
"button:has(i:text('add_2'))",
"[role='button']:has(i.google-symbols:text('add_2'))",
r"button:text-matches('^\+\s+\S+$', 'i')",
# The migrated `flow.google.com` gallery renders the CTA with the ligature **`add`**,
# under a `<mat-icon>`, and renders `add_2` nowhere on that surface (measured
# 2026-09-07, denon82, control 47 ligature nodes). Class-only so one entry covers both
# carriers. Listed FIRST because without it every Tier-1 entry below misses there and
# the CTA survives only on the English text fallback — which is the anti-pattern this
# tier exists to avoid, and which fails outright on a non-EN migrated profile.
"button:has(.google-symbols:text-is('add'))",
"[role='button']:has(.google-symbols:text-is('add'))",
"button:has(.google-symbols:text-is('add_2'))",
"[role='button']:has(.google-symbols:text-is('add_2'))",
# `button:text-matches('^\+\s+\S+$', 'i')` used to sit here. It is not a valid
# Playwright selector and RAISES on every evaluation — measured twice against the live
# gallery, both runs `ERR Error`. `except Exception: continue` in the sweep swallowed
# that, so it has never matched anything on any host while costing a round trip per
# attempt. Deleted rather than repaired: the `add` anchor above is what the "+ <word>"
# regex was reaching for, and it is structural rather than a text shape.
# Tier 2 — localised text: 14 locales (EN / PT / ES / FR / DE / IT / NL /
# JA / ZH / KO / PL / RU / TR / ID).
"button:has-text('New project')", # EN
Expand Down Expand Up @@ -1578,25 +1589,12 @@ async def _enter_editor(
continue

shot_path = await _capture_debug_screenshot(page, out_dir, "debug_new_project.png")
# Name the host, not the selector. This cascade anchors on the `add_2` ligature;
# the migrated frontend renders `add` and renders `add_2` NOWHERE (measured
# 2026-09-07 on both surfaces with per-surface controls —
# docs/superpowers/spikes/2026-09-07-ligature-carrier-and-name-drift.md). So on
# that host the sweep cannot succeed, and reporting "Could not find the CTA"
# sends the reader after selector drift — remediation "check for a newer release,
# then file a frontend bug" — for a control that host does not render at all.
# Same misdiagnosis shape as a credit shortfall reported as frontend drift (#726).
if flow_host_kind(page.url) == "migrated":
raise FlowHostMigratedError(
detail=(
"Flow served this account's gallery from flow.google.com, whose "
"frontend renders no '+ New project' control gflow can drive, so a "
"project cannot be created here. This is not selector drift and it is "
"not transient — the handoff is a per-account setting. Pass --project "
"with an existing project id, which every migrated-host path requires."
f"{screenshot_clause(shot_path)}"
)
)
# NO migrated-host branch here. #739 added one asserting that
# flow.google.com "renders no '+ New project' control gflow can drive". That was an
# UNPROVEN NEGATIVE, generalised from a sweep of two other surfaces, and a live run
# disproved it in one click: the CTA is there, carries the `add` ligature, and the
# Tier-1 entry above now matches it. Reaching this line on a migrated host means the
# anchor missed — which is selector drift, exactly what the message says.
msg = (
f"Could not find 'New project' CTA on Flow gallery. "
f"URL: {page.url}.{screenshot_clause(shot_path)}"
Expand Down
52 changes: 52 additions & 0 deletions tests/api/transports/test_ligature_carrier.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@

from gflow_cli.api.transports.ui_automation import (
IMAGE_MODEL_PICKER_TRIGGER,
NEW_PROJECT_SELECTORS,
PROMPT_FORMAT_SELECTORS,
SUBMIT_BUTTON_SELECTORS,
)

Expand Down Expand Up @@ -148,3 +150,53 @@ async def test_shipped_image_model_picker_resolves_on_both_carriers(self, page:
"silent: the picker is best-effort and generation proceeds on whatever tier the "
"editor opened at"
)


# Every selector gflow sweeps at runtime. A cascade entry that cannot be PARSED is worse
# than one that misses: the sweep's `except Exception: continue` swallows it silently, so
# it costs a round trip on every attempt and can never match on any host.
_SHIPPED_CASCADES = [
("NEW_PROJECT_SELECTORS", NEW_PROJECT_SELECTORS),
("SUBMIT_BUTTON_SELECTORS", SUBMIT_BUTTON_SELECTORS),
("PROMPT_FORMAT_SELECTORS", PROMPT_FORMAT_SELECTORS),
("IMAGE_MODEL_PICKER_TRIGGER", (IMAGE_MODEL_PICKER_TRIGGER,)),
]


@pytest.mark.asyncio
@pytest.mark.parametrize(
("name", "selectors"), _SHIPPED_CASCADES, ids=[n for n, _ in _SHIPPED_CASCADES]
)
async def test_every_shipped_selector_parses(
page: Page, name: str, selectors: tuple[str, ...]
) -> None:
"""Every entry must be a selector Playwright can evaluate — not just plausible text.

Written from a live finding: `button:text-matches('^\\+\\s+\\S+$', 'i')` sat in
NEW_PROJECT_SELECTORS and RAISED on every evaluation. The sweep caught the exception
and moved on, so it never matched anything on any host and nothing ever said so. A
unit test that pinned the regex was anchored passed the whole time, because it
inspected the string instead of running it.

`.count()` against a blank page is the cheapest thing that would have caught it: no
account, no network, no credits, and it fails loudly on an unparseable selector.
"""
await page.set_content("<html><body></body></html>")
for sel in selectors:
try:
await page.locator(sel).count()
except Exception as exc: # noqa: BLE001 - the failure IS the assertion
pytest.fail(f"{name} entry does not parse: {sel!r} -> {type(exc).__name__}: {exc}")


@pytest.mark.asyncio
async def test_the_parse_guard_can_actually_fail(page: Page) -> None:
"""A guard that only ever passes proves nothing.

This is the exact string that shipped in NEW_PROJECT_SELECTORS and raised on every
evaluation. If Playwright ever starts accepting it, this test fails and the guard
above has quietly stopped discriminating.
"""
await page.set_content("<html><body><button>+ Project</button></body></html>")
with pytest.raises(Exception): # noqa: B017, PT011 - any parse failure is the point
await page.locator(r"button:text-matches('^\+\s+\S+$', 'i')").count()
146 changes: 55 additions & 91 deletions tests/api/transports/test_new_project_cta_migrated.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,108 +1,72 @@
"""The migrated gallery has no "+ New project" CTA — say that, don't blame the selector.
"""The migrated gallery DOES have a "+ New project" CTA — it just carries a different ligature.

`NEW_PROJECT_SELECTORS` anchors on the ``add_2`` Material Symbols ligature. The migrated
``flow.google.com`` frontend renders **`add`**, and renders ``add_2`` nowhere at all —
measured 2026-09-07 on `denon82`, both surfaces, with per-surface controls
(``docs/superpowers/spikes/2026-09-07-ligature-carrier-and-name-drift.md``). So on that host
the sweep walks every selector, matches nothing, and raises:
This file exists as the correction to a mistake, and keeps the measurement that settled it.

RuntimeError: Could not find 'New project' CTA on Flow gallery. URL: https://flow.google.com/...
#739 shipped a guard asserting that `flow.google.com` "renders no '+ New project' control
gflow can drive", raising `FlowHostMigratedError` when the cascade missed. That was an
**unproven negative**, generalised from a sweep of two OTHER surfaces (the project composer
and the character editor) to a third that was never probed. A live run disproved it in one
click: `_enter_editor` with no project id created a project on the migrated host.

That message is wrong in the way this repo keeps having to fix. It reports a **selector**
problem — the remediation for which is "check for a newer gflow-cli release, then file a
frontend bug" — when the truth is that this host does not render that control in any form.
It is the same misdiagnosis shape as a credit shortfall reported as frontend drift (#726):
a real, unfixable-by-the-reader error message pointing at the wrong culprit.
Measured on the gallery afterwards (denon82, control 47 ligature nodes, `$0`):

No production path reaches this today: `migrated_can_serve` refuses without a `project_id`,
`ensure_editor` navigates straight to the project URL, and `character create` requires
`--project`. Those guards are what make the `add_2` name drift harmless *right now* — and
they are exactly what a future port relaxes. A spike hit this on 2026-09-07 and got the
misleading message; the next person to un-guard a path would get it too.
add* ligatures {"mat-icon|add": 1} <- the CTA carries `add`
[0] add_2 / <i> 0
[1] add_2 / i 0
[2] add_2 / role 0
[4] has-text('New project') 1 <- the ONLY match

So the CTA is present, and before this fix it survived purely on the **English text
fallback** — the anti-pattern Tier 1 exists to avoid, and one that fails outright on a
non-EN migrated profile. The guard would then have told that user "this host has no such
control, pass --project", which is false and is a dead end.

The fix is a Tier-1 anchor on `add`, class-only so it covers both carriers. These tests pin
that the structural tier — not localised text — is what matches.
"""

from __future__ import annotations

from typing import Any
from unittest.mock import AsyncMock, MagicMock

import pytest

from gflow_cli.api.transports.ui_automation import NEW_PROJECT_SELECTORS, UiAutomationTransport
from gflow_cli.errors import FlowHostMigratedError

_MIGRATED_GALLERY = "https://flow.google.com/?hl=en"
_LABS_GALLERY = "https://labs.google/fx/en/tools/flow"


def _gallery_page(url: str) -> MagicMock:
"""A gallery page where NOTHING matches — the state the migrated host produces."""
page = MagicMock()
page.url = url
page.goto = AsyncMock()
page.wait_for_timeout = AsyncMock()
page.wait_for_url = AsyncMock()
page.screenshot = AsyncMock(return_value=b"")

def _locator(_sel: str) -> Any:
loc = MagicMock()
loc.first = loc
loc.wait_for = AsyncMock(side_effect=Exception("not visible"))
loc.click = AsyncMock()
loc.count = AsyncMock(return_value=0)
return loc

page.locator = MagicMock(side_effect=_locator)
return page

from gflow_cli.api.transports.ui_automation import NEW_PROJECT_SELECTORS

def _transport(page: MagicMock) -> UiAutomationTransport:
t = UiAutomationTransport()
t._setup_done = True # type: ignore[attr-defined]
t._page = page # type: ignore[attr-defined]
# The gallery preamble is not what this test is about.
t._settle_if_redirecting = AsyncMock() # type: ignore[attr-defined]
t._bypass_onboarding = AsyncMock() # type: ignore[attr-defined]
t._dismiss_blocking_overlays = AsyncMock(return_value=False) # type: ignore[attr-defined]
t._require_unblocked = AsyncMock() # type: ignore[attr-defined]
return t

class TestNewProjectCtaAnchors:
def test_tier_one_covers_the_migrated_add_ligature(self) -> None:
"""The measured ligature on the migrated gallery is `add`, not `add_2`."""
tier1 = [s for s in NEW_PROJECT_SELECTORS if "google-symbols" in s]
assert any(":text-is('add')" in s for s in tier1), (
"the migrated gallery CTA carries the `add` ligature (measured 2026-09-07); "
"without a Tier-1 entry for it the CTA is reachable only by English text"
)

class TestNewProjectCtaOnMigratedHost:
@pytest.mark.asyncio
async def test_migrated_gallery_names_the_host_not_the_selector(self) -> None:
page = _gallery_page(_MIGRATED_GALLERY)
t = _transport(page)

with pytest.raises(FlowHostMigratedError) as excinfo:
await t._enter_editor(page) # type: ignore[attr-defined]

detail = str(excinfo.value)
assert "flow.google.com" in detail
assert "--project" in detail, "the error must name the way forward, not just the wall"

@pytest.mark.asyncio
async def test_labs_gallery_still_reports_a_missing_cta(self) -> None:
"""On labs the CTA genuinely should be there, so its absence IS selector drift.

The migrated branch must not swallow the real failure mode it was carved out of.
"""
page = _gallery_page(_LABS_GALLERY)
t = _transport(page)

with pytest.raises(RuntimeError, match="Could not find 'New project' CTA") as excinfo:
await t._enter_editor(page) # type: ignore[attr-defined]

assert not isinstance(excinfo.value, FlowHostMigratedError)
def test_the_add_anchors_are_carrier_agnostic(self) -> None:
"""Class-only, so one entry covers labs `<i>` and migrated `<mat-icon>` alike."""
for sel in NEW_PROJECT_SELECTORS:
if ":text-is('add')" in sel or ":text-is('add_2')" in sel:
assert "i.google-symbols" not in sel, (
f"{sel!r} is tag-qualified — it cannot match the migrated <mat-icon>"
)

def test_the_cta_cascade_still_anchors_on_add_2(self) -> None:
"""Pins WHY the migrated branch exists, so the two cannot drift apart.
def test_a_structural_anchor_precedes_every_localised_text_entry(self) -> None:
"""Tier 1 before Tier 2, or the CTA is found by locale and breaks off-English.

If someone re-anchors this cascade on `add` (the ligature the migrated host
actually renders), this test fails and the branch above should be revisited
rather than silently kept.
This is the property the bug violated in practice: every structural entry missed on
the migrated host, so `button:has-text('New project')` was doing the work.
"""
assert any("add_2" in s for s in NEW_PROJECT_SELECTORS), (
"the migrated-host branch exists because this cascade anchors on `add_2`, "
"which flow.google.com does not render — re-anchoring it changes that premise"
first_text = next(
(i for i, s in enumerate(NEW_PROJECT_SELECTORS) if "has-text(" in s), None
)
first_struct = next(
(i for i, s in enumerate(NEW_PROJECT_SELECTORS) if "google-symbols" in s), None
)
assert first_struct is not None, "the cascade must carry a structural tier"
assert first_text is not None
assert first_struct < first_text

@pytest.mark.parametrize("sel", NEW_PROJECT_SELECTORS)
def test_every_entry_is_a_valid_selector_shape(self, sel: str) -> None:
"""`:has-text()` is invalid inside `:has()` — the repo's documented trap."""
if ":has(" in sel:
assert ":has-text(" not in sel
Loading