fix(migrated): say what was true when a click never lands (#776) - #778
Merged
Conversation
…ible-but-unclickable
0/3 on the overlay hypothesis — body{pointer-events:none} never occurred and the
click landed in 65-130ms every run, so this settles nothing about #776 and is
recorded as unmeasured, not as transience.
Three things it did measure:
- _dismiss_dialog runs before Angular mounts the composer, 3/3 (trigger appears
1479-3143ms after domcontentloaded, the check fires at ~0ms). The miss is
structural, not flaky.
- a healthy click on this control costs 65-130ms against a 5000ms budget, so
#776's expiry is a control that never became actionable.
- the locale-settle error the reporter asked about reproduced on all 3 runs while
the click still landed - it is #643, and it is not sufficient to cause #776.
Rung 1 also found #593's pointer-events measurement was taken on labs.google;
migrated_composer.py:744 calling the migrated dialog '#593's twin' is asserted,
not measured.
Refs #776
video r2v on flow.google.com reached migrated.editor_ready and died 5.039s later as a bare Playwright TimeoutError - exit 1, no locator, no cause, no MP4. By elimination that is _open_pane's trigger.click(timeout=5000): the wait_for(visible) one line above IS guarded and would have raised exit 23, so the control was visible and the click expired. #752 finding #7 predicted this at this exact function before #776 was filed. Its count()->visibility half was fixed; the click half was not, leaving a comment that describes the failure the next line went on producing. It reads, it does not diagnose. Two causes were live and neither could be measured: Flow's announcement overlay (#593, measured on labs.google, never on this host - 0/3 in today's spike) and a mid-run agent-mode flip. A guard built on either would answer confidently and be wrong half the time (#770 is the precedent). So on a timeout the driver reads Playwright's four actionability conditions back and reports the ones that fired; when every reading is healthy it says so, which eliminates three and leaves 'stable' rather than inventing a fourth. - costs nothing when healthy: the read runs only in the except branch, the rule raise_if_known_landing already states - MCP gains more than the CLI: a non-GFlowError on the queued path shipped 'detail: sha256:...'; the typed error routes it to the Problem Details branch - four sites with a named reason each, not all nineteen - the occluder report is a closed allowlist (tag + <=3 framework class tokens); typing the error moves the text from hashed telemetry to a message printed raw, and a signed-in Flow page carries the account email on exactly those elements - retryable unchanged: preserved, not measured Closes #776
…e council caught Council D9 found a Blocker I had put on my own predict list and then dropped: UiSelectorDriftError's docstring still said 'finds no matching element', which the new raise site makes false - it now also fires when the selector DOES match and the element will not take the interaction. - errors.py: the class covers two shapes, and the detail has to say which - KNOWN_ISSUES: the lookup table documented 6 of the 8 messages the code can emit; the missing two were 'not rendered' and 'answers no hit test' - E2E_TESTING: a second worked example, chosen because it fails DIFFERENTLY from the first - a client-side redirect vs Playwright's actionability gate - tests: one offline case per reading. CI's coverage run excludes -m e2e, so a branch proven only in the browser reads as dead code to Sonar's new-code gate - the exact way PR #777 went red at 70% - tests/worker: the MCP twin A/B - a bare TimeoutError reaches an agent as 'detail: sha256:...', the typed one as problem details with exit 23 Refs #776
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThe migrated composer now converts Playwright click timeouts into structured ChangesMigrated click attribution
Estimated code review effort: 4 (Complex) | ~45 minutes Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant MigratedComposer
participant Playwright
participant FlowPage
participant MCP
MigratedComposer->>Playwright: click migrated locator
Playwright-->>MigratedComposer: TimeoutError
MigratedComposer->>FlowPage: read actionability state
FlowPage-->>MigratedComposer: redacted structural diagnostics
MigratedComposer->>MCP: raise UiSelectorDriftError
MCP-->>MigratedComposer: return exit 23 problem details
✨ 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 |
… told twice Council D14 measured 64.5% of this change's added lines as non-code prose, against the 35% issue #759 already named as a problem in this same file. Fair hit - three of the passages were the CHANGELOG's content retold at the call site, and one was an exact duplicate of the comment 80 lines above it. - _click's docstring: the #593/#752/#770 archaeology is the CHANGELOG's job - the redaction comment above the redact_sensitive_text return restated the module-level one verbatim - the _open_pane call site told the #752 story a third time - SUBMIT_BUTTON's three-line comment for a one-line constant D1 also found a real over-report: the JS only hit-tests when the box has a nonzero width and height, so an unrendered element ALWAYS comes back hit_testable=false and occluder=null. Two independent ifs then said "it is not rendered" and "it answers no hit test" about one fact. Now one chain for "can a pointer reach it", ordered most specific first, with enabled-ness and a page-wide block kept as the separate axes they are - chaining those would have hidden a disabled control behind whatever covered it. Two tests added for the invariants the real JS enforces, which the earlier parametrized cases violated by holding hit_testable=True on an unrendered element. Refs #776
ffroliva
force-pushed
the
bugfix/776-attributable-click-timeouts
branch
from
September 10, 2026 16:22
d016a5c to
d1b9195
Compare
… that no longer works Council D4 caught a false statement in my own docstring: it said the queued MCP path raw-slices detail to 500 chars "while the CLI path does not". It does. redact_sensitive_text IS redact_error_detail, which truncates at the raise site, so every surface sees the same cap. Locator-first ordering is still right, for the simpler reason that the cut happens at the source - fixed in both the method docstring and the test that repeated it. Also: - tests/worker: _fail_r2v_with enqueues a t2v task. The name described the reporter's command, not the fixture. - tests/api/transports/test_migrated_composer.py defines a local PlaywrightTimeoutError that shadows the real class for that whole 2000-line file. Since #776 the driver catches the REAL one, so a future click-timeout test written with that file's dominant fake would silently exercise nothing and say so nowhere. Documented at the definition, pointing at the file that does it correctly. Refs #776
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
gflow video r2vonflow.google.comreachedmigrated.editor_readyand died 5.039 s later as a bare PlaywrightTimeoutError— exit 1, no locator, no cause, no MP4, 0 credits spent. The reporter asked for two things: fail before submitting with a diagnostic naming the failed action, and preserve the failing locator before page teardown.Root cause, by elimination.
_open_panemakes exactly two calls betweeneditor_readyand the next log line::1013trigger.wait_for(state="visible", timeout=5000)UiSelectorDriftError, exit 23, names the anchor:1031trigger.click(timeout=5000)TimeoutError, exit 1, names nothingThe reporter got a bare
TimeoutError, so it was the click — and their own timestamps confirm it arithmetically:editor_ready13:01:03.560 →incident.capture_started13:01:08.599 = 5.039 s, await_forthat returned immediately plus a 5000 ms click timeout.#752 finding #7 predicted this, at this function, before #776 was filed:
The
count()→visibility half was fixed. The click half was not — leaving a comment at:1008-1011that describes the failure the next line went on producing.It reads; it does not diagnose
Two causes were live and neither could be measured:
ci-probetoday readbody{pointer-events}asautoin 159/159 samples, including while Flow's own settings pane was open. Angular CDK blocks with a.cdk-overlay-backdropelement, not by muting the body.A guard built on either would answer confidently and be wrong about half the time. #770 is the live precedent for what that costs a reporter.
So on a timeout the driver reads Playwright's four actionability conditions back — agent chip,
hidden/disabled/not-rendered, body pointer-events, and a hit test naming what is on top — and reports the ones that fired. When every reading is healthy it says exactly that, which eliminates three conditions and points at stable, rather than inventing a fourth.Costs nothing on a healthy run: the read happens only in the
exceptbranch — the ruleraise_if_known_landingalready states, "a guard placed ahead of the probe deletes the evidence that would correct it."MCP gains more than the CLI
A non-
GFlowErroron the queued path tookdaemon.py'selsebranch and shipped"detail": "sha256:…"— a hash, not even the exception class. An agent could not tell a covered button from a dead network. The typed error routes it to theisinstance(exc, GFlowError)branch instead: full RFC 9457 problem details,exit_code: 23, locator intact. Proven by an A/B pair intests/worker/test_daemon.pyrunning the sameprocess_taskbranch with both exception shapes.Scope
Four sites, each with a named reason — not all nineteen (open #759 flags this file for narrative bloat):
_open_panesend_prompt_close_pane's own docstring records this click failing exactly this waysubmit_and_observe/submit_images_and_observeSecurity
Typing the error removes a privacy net: a bare exception is SHA-256-hashed by
_handle_unhandled_error, whileGFlowError.detailis printed raw to the console, shipped through structlog, emitted under--json, and invited into a GitHub issue by the class's own remediation hint. PR #777 fixed this exact bug class one surface over, two hours earlier.So the occluder report is a closed allowlist — tag name plus at most three
cdk-/mat-/mdc-/flow-class tokens. NeverouterHTML,textContent,aria-label,title,alt,srcorhref. Every return path also passesredact_sensitive_text(). A live e2e scenario plants an account email inaria-label/title/altand a signed URL insrcon the covering element and asserts neither reaches the message.retryableis unchanged — preserved, not measured. The condition did not reproduce, and a flag that moves as a side effect of retyping is a claim nobody made.Test plan
tests/features/click_attribution.feature+tests/e2e/test_click_attribution_bdd.py. Went RED 5/6 first (the one pass was the A/B control), now 6 passed. Each scenario breaks a different actionability condition for real: a stackeddivthat intercepts pointers, a CSS animation that never lets the box settle, a pressed agent chip.tests/api/transports/test_click_attribution.py. One per reading, because CI's coverage run excludes-m e2e, so a branch proven only in the browser reads as dead code to Sonar's new-code gate (how fix(security): strip OAuth query params from chooser error messages #777 went red at 70%).tests/worker/test_daemon.py.ruff·ruff format·uv run pyright src→ 0 errors.docs/superpowers/spikes/2026-09-10-migrated-click-blocked.md.What this does NOT establish
I did not measure what blocked the reporter's click. The overlay hypothesis is 0/3 on
ci-probe— unmeasured, not disproven, and recorded that way. The fix is built so it does not need the answer: the next occurrence reports its own cause instead of producing another unattributable timeout.Also measured: the reporter's
account_locale_lang_unchanged … reason=Errorreproduced on 3/3 spike runs while the click landed normally. It is real, it is #643, and it is not #776's cause.Closes #776
Summary by CodeRabbit
Bug Fixes
Documentation