fix: improving webviews#172
Open
gmegidish wants to merge 45 commits into
Open
Conversation
Defines the protocol-layer interfaces for webview support. MobilewrightDriver gains an optional webViewBridge property so existing drivers compile unchanged.
Adds { kind: 'webview' } to LocatorStrategy with exact type matching for
WKWebView, XCUIElementTypeWebView, android.webkit.WebView, RCTWebView, and
RNCWebView. Includes unit tests for all five types, chaining, and empty results.
WebViewLocator extends Locator and adds page() which attaches to the bridge by matching the native webview index to bridge.listWebViews(). first/last/nth are overridden to preserve WebViewLocator type; other chaining methods return plain Locator. Page is a minimal stub to be filled in step 4.
Page exposes all v1 locator factories (getByRole, getByText, getByLabel, getByPlaceholder, getByTestId, getByAltText, getByTitle, locator) and page-level methods (url, title, goto, reload, evaluate, waitForURL, waitForLoadState, content, close). WebLocator defines WebLocatorStrategy and factory/collection methods; actions and queries come in step 5.
…implementation Introduces dom-selector-engine.ts — a browser-injectable JS string defining window.__mw with W3C-compliant ARIA role computation, accessible name algorithm, text matching (innermost-match semantics), and label detection. buildFindAll() now delegates to window.__mw.* instead of naive inline CSS/JS guesses. Page gains a static attach() factory that injects the engine once per session. WebLocator gains full query methods (isVisible, textContent, inputValue, getAttribute, count, waitFor, boundingBox) and action methods (click, fill, type, press, focus, hover, scrollIntoViewIfNeeded).
expect(page).toHaveURL/toHaveTitle — polls page.url()/title() with auto-retry. expect(webLocator).toBeVisible/Hidden/Enabled/Disabled/Checked, toHaveText, toContainText, toHaveValue, toHaveAttribute, toHaveCount — all with .not and configurable timeout. Exports Page, WebLocator, WebViewLocator, WebLocatorStrategy from the package index.
WebLocator gains getText()/getValue() aliases so LocatorAssertions can drive it. LocatorAssertions fields/helpers widened to LocatorLike interface and made protected. toHaveCount moved to LocatorAssertions (works for both locator types). WebLocatorAssertions now extends LocatorAssertions and only adds toHaveAttribute.
75 tests across page.test.ts and web-locator.test.ts covering Page.attach(), all page-level methods, locator factory JS generation, state queries, value queries, actions, waitFor, expect(page) and expect(webLocator) matchers with .not negation. Also fixes two bugs found during testing: - LocatorAssertions.not now uses this.constructor so subclasses (WebLocatorAssertions) get the correct type back without needing to override not - toHaveCount was missing negation in its predicate, causing not.toHaveCount to time out instead of resolving immediately
…idy selector engine
…r negation in toHaveAttribute
… and attribute negation
…laywright's injected engine
…ion, nav re-injection, stdio drain, verbose diagnostics)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces WebView support to mobilewright by integrating Playwright's injected engine and creating a complete abstraction for in-page interaction. The work spans protocol contracts (WebViewSession, WebViewBridge), a mobilecli driver implementation that provides RPC-based webview operations, Playwright engine integration for selector building and in-page script injection, WebLocator for DOM querying and interaction via the injected engine, Page class as a Playwright-like wrapper around WebViewSession, WebViewLocator for native webview selection, extended expect() matchers for Page and WebLocator assertions, and a comprehensive E2E conformance suite validating both Playwright and mobilewright runtimes with shared spec implementations. 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| private actOnFirst(action: string, what: string): Promise<void> { | ||
| const notFound = JSON.stringify(`${what}: element not found`); | ||
| return this.session.evaluate<void>( | ||
| `(() => { const el = ${this.firstEl()}; if (!el) { throw new Error(${notFound}); } ${action} })()`, |
| private actOnFirst(action: string, what: string): Promise<void> { | ||
| const notFound = JSON.stringify(`${what}: element not found`); | ||
| return this.session.evaluate<void>( | ||
| `(() => { const el = ${this.firstEl()}; if (!el) { throw new Error(${notFound}); } ${action} })()`, |
| private actOnFirst(action: string, what: string): Promise<void> { | ||
| const notFound = JSON.stringify(`${what}: element not found`); | ||
| return this.session.evaluate<void>( | ||
| `(() => { const el = ${this.firstEl()}; if (!el) { throw new Error(${notFound}); } ${action} })()`, |
| private async readElementState(state: 'enabled' | 'checked', timeout: number, what: string): Promise<boolean> { | ||
| const sel = JSON.stringify(this.selector); | ||
| const stateArg = JSON.stringify(state); | ||
| const js = `(() => { const is = window.__mwInjected; const el = is.querySelector(is.parseSelector(${sel}), document, true); if (!el) { return null; } return is.elementState(el, ${stateArg}).matches; })()`; |
| private async readElementState(state: 'enabled' | 'checked', timeout: number, what: string): Promise<boolean> { | ||
| const sel = JSON.stringify(this.selector); | ||
| const stateArg = JSON.stringify(state); | ||
| const js = `(() => { const is = window.__mwInjected; const el = is.querySelector(is.parseSelector(${sel}), document, true); if (!el) { return null; } return is.elementState(el, ${stateArg}).matches; })()`; |
|
|
||
| async isVisible(opts?: { timeout?: number }): Promise<boolean> { | ||
| const sel = JSON.stringify(this.selector); | ||
| const js = `(() => { const is = window.__mwInjected; const el = is.querySelector(is.parseSelector(${sel}), document, true); if (!el) { return false; } return is.elementState(el, 'visible').matches; })()`; |
| const list = JSON.stringify(states); | ||
| await retryUntil( | ||
| () => this.session.evaluate<boolean>( | ||
| `(async () => { const is = window.__mwInjected; const el = is.querySelector(is.parseSelector(${sel}), document, true); if (!el) { return false; } const missing = await is.checkElementStates(el, ${list}); return missing === undefined; })()`, |
| const list = JSON.stringify(states); | ||
| await retryUntil( | ||
| () => this.session.evaluate<boolean>( | ||
| `(async () => { const is = window.__mwInjected; const el = is.querySelector(is.parseSelector(${sel}), document, true); if (!el) { return false; } const missing = await is.checkElementStates(el, ${list}); return missing === undefined; })()`, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (4)
packages/driver-mobilecli/src/driver.test.ts (1)
249-268: 💤 Low valueConsider verifying params for all navigation calls.
The test verifies that the correct RPC methods are called and checks params for
gotoandgoBack, but doesn't verify params forgoForwardandreload. Since all methods follow the same pattern, this works, but verifying all params would provide more complete coverage.✅ Optional improvement
// deviceId is injected by the driver, id by the session. expect(calls[0].params).toEqual({ deviceId: SIMULATOR_DEVICE_ID, id: 'wv-1', url: 'https://example.com/' }); expect(calls[1].params).toEqual({ deviceId: SIMULATOR_DEVICE_ID, id: 'wv-1' }); + expect(calls[2].params).toEqual({ deviceId: SIMULATOR_DEVICE_ID, id: 'wv-1' }); + expect(calls[3].params).toEqual({ deviceId: SIMULATOR_DEVICE_ID, id: 'wv-1' });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/driver-mobilecli/src/driver.test.ts` around lines 249 - 268, The test 'navigation methods call the matching RPC with the device and webview ids' currently asserts RPC methods and params for goto and goBack only; add assertions for the remaining navigation calls by verifying calls[2].params and calls[3].params equal the expected objects (include deviceId: SIMULATOR_DEVICE_ID and id: 'wv-1') so that session.goForward and session.reload are explicitly checked; locate this in the test using createDriverWithSession, recordRpc, driver.webViewBridge.attachWebView and the session.goto/goBack/goForward/reload calls and add the two expect(...) assertions mirroring the goBack assertion.e2e/src/webview-injected-click.test.ts (1)
13-13: 💤 Low valueConsider using the
pageWithBodyhelper for consistency.This test constructs the data URL inline, while the conformance tests use the
pageWithBodyhelper fromharness.ts. Using the helper would:
- Add proper charset declaration
- Provide complete HTML structure (doctype, meta tags)
- Improve consistency across the test suite
♻️ Refactor to use pageWithBody
+import { pageWithBody } from './conformance/harness.js'; + // ... - await page.goto('data:text/html,<button onclick="document.title=\'clicked\'">Sign in</button>'); + await page.goto(pageWithBody('<button onclick="document.title=\'clicked\'">Sign in</button>'));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/src/webview-injected-click.test.ts` at line 13, Replace the inline data URL navigation with the shared helper: import and call pageWithBody(page, '<button onclick="document.title=&`#39`;clicked&`#39`;">Sign in</button>') instead of await page.goto(...); this will add the proper charset and full HTML structure used by the conformance suite—locate the test in e2e/src/webview-injected-click.test.ts and replace the await page.goto(...) call with a pageWithBody invocation and add the import for pageWithBody from harness.ts.e2e/src/conformance/harness.ts (1)
19-19: 💤 Low valueConsider adding complete HTML structure.
The generated document is missing
<html>opening and closing tags. While browsers are forgiving and will auto-close tags, a complete structure would be more robust:- const doc = `<!doctype html><meta charset="utf-8"><body>${bodyHtml}</body>`; + const doc = `<!doctype html><html><head><meta charset="utf-8"></head><body>${bodyHtml}</body></html>`;This ensures the fixture is valid HTML5 and avoids relying on browser error-recovery.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/src/conformance/harness.ts` at line 19, The HTML fixture built in the e2e harness assigns doc to a string missing the <html> tags; update the doc construction (the const doc assignment) to wrap the existing meta and bodyHtml inside explicit <html> and </html> tags (for example: <!doctype html><html><meta charset="utf-8"><body>...</body></html>) so the generated document has a complete HTML5 structure.packages/mobilewright-core/src/playwright-engine.ts (1)
19-38: Add a CI regression check forplaywright-coreinternal paths used byplaywright-engine.ts
playwright-engine.tsrelies on type assertions over internalplaywright-coremodules (lib/generated/injectedScriptSource.jsandlib/utils/isomorphic/locatorUtils.js). A small runtime probe in CI that asserts the files exist and the expected exports are present (at leastinjectedScriptSource.sourceandlocatorUtils.getByRoleSelector; ideally the other re-exported selectors too) would catch silent breakage on dependency bumps.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/mobilewright-core/src/playwright-engine.ts` around lines 19 - 38, Add a CI regression probe that verifies the runtime presence and expected exports of the internal playwright-core modules used by playwright-engine.ts: require and assert that 'lib/generated/injectedScriptSource.js' exports an object with a string property injected.source (referenced as INJECTED_SOURCE/injected.source) and that 'lib/utils/isomorphic/locatorUtils.js' exports the selector functions used (locatorUtils.getByRoleSelector, locatorUtils.getByTextSelector, locatorUtils.getByLabelSelector, locatorUtils.getByPlaceholderSelector, locatorUtils.getByAltTextSelector, locatorUtils.getByTitleSelector and locatorUtils.getByTestIdSelector); implement this as a simple CI-only Node script or test that fails the build when the files are missing or the named exports are not functions/strings so dependency bumps are caught early.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-06-05-webview-playwright-parity-slice1.md`:
- Around line 138-142: The fenced NOTICE block (the triple-backtick block
containing the Playwright copyright/license text) lacks a language tag which
triggers markdownlint MD040; add a language identifier such as "text" (i.e.,
change the opening fence from ``` to ```text) for that fenced code block so the
linter recognizes it as a plain-text block and the MD040 warning is resolved.
In `@docs/superpowers/plans/2026-06-05-webview-playwright-parity-slice2.md`:
- Line 40: The docs show getByTextSelector as taking a positional exact boolean
(getByTextSelector('Hello', true)) but the adapter/interface actually expects an
options object (e.g., { exact?: boolean }); update the documented signature and
all examples (lines referenced around getByTextSelector and the selector-builder
signatures) to use the options-object form to match the adapter contract, or
alternatively change the adapter's function signature to accept a positional
exact parameter—ensure getByTextSelector, the selector-builder signatures, and
any examples are consistent with the adapter shape you choose.
In `@docs/superpowers/plans/2026-06-06-webview-conformance-suite.md`:
- Around line 141-143: The documentation uses ctx.screen.getByText('Webview')
which mismatches the conformance harness label; update the call so the visible
text matches the implemented harness (use 'Web View' instead of 'Webview') where
webviewButton is retrieved (ctx.screen.getByText) and ensure any subsequent
usage of ctx.screen.getByWebView() remains consistent with that label change.
In
`@docs/superpowers/specs/2026-06-05-webview-playwright-parity-slice2-design.md`:
- Around line 36-43: Update the spec lines for getByTextSelector,
getByLabelSelector, getByPlaceholderSelector, getByAltTextSelector,
getByTitleSelector and getByTestIdSelector to reflect the adapter contract which
uses an options object rather than a positional boolean: replace any mention of
a positional `exact?` argument with `{ exact?: boolean }` (and adjust example
selector call shapes accordingly), and also update the related note about
`exact` (lines ~119-120) to state that text-like builders accept an options
object instead of a positional boolean.
In `@docs/superpowers/specs/2026-06-06-webview-conformance-suite-design.md`:
- Around line 27-29: The spec and harness use inconsistent button labels
("Webview" vs "Web View"), causing brittle conformance steps; update the spec
text entries (e.g., the "Navigation to the webview" section and the other
occurrences noted around the doc) to use the exact same label string the harness
expects ("Web View") and/or update the harness to match the canonical spec
label; ensure all occurrences (including the navigation instruction that
references screen.getByWebView().page() and the platform note mentioning
WKWebView) are normalized to the chosen label so manual runs and tests reference
an identical button label.
In `@e2e/src/webview-injected-click.test.ts`:
- Line 4: The test hardcodes APP_ID ('com.example.webviewdemo') which may not
exist on target devices; change APP_ID in e2e/src/webview-injected-click.test.ts
to use the existing PLAYGROUND_APP constant or make APP_ID configurable (e.g.,
read from test config or env) and update device.launchApp(APP_ID) accordingly;
also add a one-line comment near APP_ID explaining that the app must be
installed and support data: goto when a non-PLAYGROUND app is used.
In `@packages/mobilewright-core/src/expect.ts`:
- Around line 471-490: The catch-all in toHaveURL and toHaveTitle currently
converts read errors into an empty string so negated assertions can spuriously
pass; update both methods to stop swallowing errors: in the retryAssertion
"actual" callback, capture successful value into `last` and capture any thrown
Error into a new `lastError` variable instead of setting `last = ''`, then
rethrow the error (or let it bubble) so retryAssertion treats it as a poll
failure; finally update the timeout message (the lambda passed as last arg) to
include `lastError` details when present so the final failure reports the read
error. Reference: methods toHaveURL, toHaveTitle, retryAssertion, this.page.url
/ this.page.title, matches, DEFAULT_TIMEOUT, and this._wrapAssertion.
In `@packages/mobilewright-core/src/web-expect-matcher.ts`:
- Around line 14-20: The FrameExpectParams.type currently exposes
expectedValue?: unknown which over-promises for JSON.stringify transport; update
FrameExpectParams and any similar fields (lines around 43-50) to a JSON-safe
type (e.g., JsonSerializable | undefined) or validate/convert expectedValue
before sending in buildExpectEvaluate so values like undefined, BigInt, NaN,
Infinity are handled/serialized deterministically; specifically, change the
public API/type of expectedValue and add a serializer/validator in
buildExpectEvaluate (and handle toHaveJSProperty caller paths) to either reject
unsupported values or convert them to a Playwright-style representation prior to
JSON.stringify.
In `@packages/mobilewright-core/src/webview-locator.ts`:
- Around line 61-67: The code silently falls back to index 0 when no native
webview nodes are found, causing .last() / .nth(i) to attach bridgeWebviews[0]
incorrectly; update the index-resolution in the webview selection logic (around
getViewHierarchy, queryAll, allNativeWebviews, bridgeWebviews, and the
.last()/.nth(i) usage) to fail fast or handle the node-less case: if
allNativeWebviews.length === 0 then either require bridgeWebviews.length === 1
and use that single entry, or throw a descriptive error indicating ambiguous
webview selection; ensure this guard is applied in both the block around
getViewHierarchy (lines ~61–67) and the similar logic at lines ~88–95 so
selection never silently defaults to index 0.
- Around line 97-100: Wrap the Page.attach(session) call in a try/catch so that
if bootstrap injection fails you explicitly clean up the opened session; e.g.
after const session = await bridge.attachWebView(target.id) do try { this._page
= await Page.attach(session); this._page._stepFn = this._stepFn; return
this._page; } catch (err) { await session.close(); throw err; } so you call the
session cleanup method (session.close() or the appropriate
session.dispose()/session.disconnect() API) on failure and rethrow the error;
reference bridge.attachWebView, session, and Page.attach to locate the change.
---
Nitpick comments:
In `@e2e/src/conformance/harness.ts`:
- Line 19: The HTML fixture built in the e2e harness assigns doc to a string
missing the <html> tags; update the doc construction (the const doc assignment)
to wrap the existing meta and bodyHtml inside explicit <html> and </html> tags
(for example: <!doctype html><html><meta
charset="utf-8"><body>...</body></html>) so the generated document has a
complete HTML5 structure.
In `@e2e/src/webview-injected-click.test.ts`:
- Line 13: Replace the inline data URL navigation with the shared helper: import
and call pageWithBody(page, '<button
onclick="document.title=&`#39`;clicked&`#39`;">Sign in</button>') instead of await
page.goto(...); this will add the proper charset and full HTML structure used by
the conformance suite—locate the test in e2e/src/webview-injected-click.test.ts
and replace the await page.goto(...) call with a pageWithBody invocation and add
the import for pageWithBody from harness.ts.
In `@packages/driver-mobilecli/src/driver.test.ts`:
- Around line 249-268: The test 'navigation methods call the matching RPC with
the device and webview ids' currently asserts RPC methods and params for goto
and goBack only; add assertions for the remaining navigation calls by verifying
calls[2].params and calls[3].params equal the expected objects (include
deviceId: SIMULATOR_DEVICE_ID and id: 'wv-1') so that session.goForward and
session.reload are explicitly checked; locate this in the test using
createDriverWithSession, recordRpc, driver.webViewBridge.attachWebView and the
session.goto/goBack/goForward/reload calls and add the two expect(...)
assertions mirroring the goBack assertion.
In `@packages/mobilewright-core/src/playwright-engine.ts`:
- Around line 19-38: Add a CI regression probe that verifies the runtime
presence and expected exports of the internal playwright-core modules used by
playwright-engine.ts: require and assert that
'lib/generated/injectedScriptSource.js' exports an object with a string property
injected.source (referenced as INJECTED_SOURCE/injected.source) and that
'lib/utils/isomorphic/locatorUtils.js' exports the selector functions used
(locatorUtils.getByRoleSelector, locatorUtils.getByTextSelector,
locatorUtils.getByLabelSelector, locatorUtils.getByPlaceholderSelector,
locatorUtils.getByAltTextSelector, locatorUtils.getByTitleSelector and
locatorUtils.getByTestIdSelector); implement this as a simple CI-only Node
script or test that fails the build when the files are missing or the named
exports are not functions/strings so dependency bumps are caught early.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 02315b85-a578-4541-b425-e2bea6cbb803
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (45)
docs/superpowers/plans/2026-06-05-webview-playwright-parity-slice1.mddocs/superpowers/plans/2026-06-05-webview-playwright-parity-slice2.mddocs/superpowers/plans/2026-06-05-webview-playwright-parity-slice3.mddocs/superpowers/plans/2026-06-06-webview-conformance-suite.mddocs/superpowers/specs/2026-06-05-webview-playwright-parity-slice1-design.mddocs/superpowers/specs/2026-06-05-webview-playwright-parity-slice2-design.mddocs/superpowers/specs/2026-06-05-webview-playwright-parity-slice3-design.mddocs/superpowers/specs/2026-06-06-webview-conformance-suite-design.mde2e/package.jsone2e/src/conformance/actions.test.tse2e/src/conformance/assertions-state.test.tse2e/src/conformance/assertions-text.test.tse2e/src/conformance/assertions-web.test.tse2e/src/conformance/harness.tse2e/src/conformance/locators.test.tse2e/src/webview-injected-click.test.tspackage.jsonpackages/driver-mobilecli/package.jsonpackages/driver-mobilecli/src/driver.test.tspackages/driver-mobilecli/src/driver.tspackages/mobilewright-core/NOTICEpackages/mobilewright-core/package.jsonpackages/mobilewright-core/src/expect.tspackages/mobilewright-core/src/fake-webview-session.tspackages/mobilewright-core/src/index.tspackages/mobilewright-core/src/locator.tspackages/mobilewright-core/src/page.test.tspackages/mobilewright-core/src/page.tspackages/mobilewright-core/src/playwright-engine.test.tspackages/mobilewright-core/src/playwright-engine.tspackages/mobilewright-core/src/query-engine.test.tspackages/mobilewright-core/src/query-engine.tspackages/mobilewright-core/src/screen.tspackages/mobilewright-core/src/stackTrace.tspackages/mobilewright-core/src/web-expect-matcher.test.tspackages/mobilewright-core/src/web-expect-matcher.tspackages/mobilewright-core/src/web-expect.test.tspackages/mobilewright-core/src/web-locator.test.tspackages/mobilewright-core/src/web-locator.tspackages/mobilewright-core/src/webview-locator.test.tspackages/mobilewright-core/src/webview-locator.tspackages/mobilewright/src/index.tspackages/mobilewright/src/server.tspackages/protocol/src/driver.tspackages/protocol/src/types.ts
Comment on lines
+471
to
+490
| async toHaveURL(url: string | RegExp, opts?: ExpectOptions): Promise<void> { | ||
| return this._wrapAssertion('toHaveURL', async () => { | ||
| let last = ''; | ||
| await retryAssertion( | ||
| async () => { try { last = await this.page.url(); } catch { last = ''; } return last; }, | ||
| (current) => this.matches(url instanceof RegExp ? url.test(current) : current === url), | ||
| opts?.timeout ?? DEFAULT_TIMEOUT, | ||
| () => `Expected page URL to ${this.negated ? 'not ' : ''}match "${url}", but got "${last}"`, | ||
| ); | ||
| }); | ||
| } | ||
|
|
||
| async toHaveTitle(title: string | RegExp, opts?: ExpectOptions): Promise<void> { | ||
| return this._wrapAssertion('toHaveTitle', async () => { | ||
| let last = ''; | ||
| await retryAssertion( | ||
| async () => { try { last = await this.page.title(); } catch { last = ''; } return last; }, | ||
| (current) => this.matches(title instanceof RegExp ? title.test(current) : current === title), | ||
| opts?.timeout ?? DEFAULT_TIMEOUT, | ||
| () => `Expected page title to ${this.negated ? 'not ' : ''}match "${title}", but got "${last}"`, |
There was a problem hiding this comment.
Don't let URL/title read failures satisfy negated assertions.
The catch { last = ''; } fallback turns operational failures into data. On .not.toHaveURL(...) / .not.toHaveTitle(...), a closed or broken session will often pass immediately because '' does not match the expected value. Treat read errors as poll failures instead, and surface the last error if the assertion times out.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/mobilewright-core/src/expect.ts` around lines 471 - 490, The
catch-all in toHaveURL and toHaveTitle currently converts read errors into an
empty string so negated assertions can spuriously pass; update both methods to
stop swallowing errors: in the retryAssertion "actual" callback, capture
successful value into `last` and capture any thrown Error into a new `lastError`
variable instead of setting `last = ''`, then rethrow the error (or let it
bubble) so retryAssertion treats it as a poll failure; finally update the
timeout message (the lambda passed as last arg) to include `lastError` details
when present so the final failure reports the read error. Reference: methods
toHaveURL, toHaveTitle, retryAssertion, this.page.url / this.page.title,
matches, DEFAULT_TIMEOUT, and this._wrapAssertion.
Comment on lines
+14
to
+20
| export interface FrameExpectParams { | ||
| expression: string; | ||
| expressionArg?: unknown; | ||
| expectedText?: ExpectedTextValue[]; | ||
| expectedNumber?: number; | ||
| expectedValue?: unknown; | ||
| isNot: boolean; |
There was a problem hiding this comment.
expectedValue?: unknown over-promises what this transport can encode.
buildExpectEvaluate() sends params through JSON.stringify(), so toHaveJSProperty(name, expected) cannot preserve several valid unknown values: undefined is omitted, NaN/Infinity become null, and BigInt throws before the matcher even runs. Either switch this path to a serializer that preserves Playwright-style JS values, or narrow/validate expectedValue to a JSON-safe type before exposing it as a public API.
Also applies to: 43-50
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/mobilewright-core/src/web-expect-matcher.ts` around lines 14 - 20,
The FrameExpectParams.type currently exposes expectedValue?: unknown which
over-promises for JSON.stringify transport; update FrameExpectParams and any
similar fields (lines around 43-50) to a JSON-safe type (e.g., JsonSerializable
| undefined) or validate/convert expectedValue before sending in
buildExpectEvaluate so values like undefined, BigInt, NaN, Infinity are
handled/serialized deterministically; specifically, change the public API/type
of expectedValue and add a serializer/validator in buildExpectEvaluate (and
handle toHaveJSProperty caller paths) to either reject unsupported values or
convert them to a Playwright-style representation prior to JSON.stringify.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/mobilewright-core/src/page.ts`:
- Around line 158-160: Preserve the exported Page.screenshot() method as a
compatibility stub: add back the Page.screenshot method (same signature as
before) that immediately throws a clear, documented error explaining screenshots
are unsupported until WebViewSession implements a capture capability (e.g.,
device.webview.screenshot RPC or native-screenshot with nativeBounds). Locate
the Page class in page.ts and reintroduce the method name Page.screenshot() so
callers keep a stable API and the runtime error references
WebViewSession/capture RPC support.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ced62d43-12bc-4ddc-86b1-bd5a21530afb
📒 Files selected for processing (1)
packages/mobilewright-core/src/page.ts
Comment on lines
+158
to
+160
| // screenshot() intentionally omitted until WebViewSession gains a capture | ||
| // capability (a device.webview.screenshot RPC, or a native-screenshot crop to | ||
| // the webview's nativeBounds). Adding it later is a non-breaking change. |
There was a problem hiding this comment.
Keep Page.screenshot() as a compatibility stub instead of removing it.
Line 160 says this is non-breaking, but removing a previously exported method is a breaking API change for existing consumers. Preserve the method signature and keep throwing until capture RPC support is implemented.
Suggested patch
// screenshot() intentionally omitted until WebViewSession gains a capture
// capability (a device.webview.screenshot RPC, or a native-screenshot crop to
// the webview's nativeBounds). Adding it later is a non-breaking change.
+ async screenshot(): Promise<Buffer> {
+ throw new Error('Page.screenshot() is not yet supported for WebViewSession');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // screenshot() intentionally omitted until WebViewSession gains a capture | |
| // capability (a device.webview.screenshot RPC, or a native-screenshot crop to | |
| // the webview's nativeBounds). Adding it later is a non-breaking change. | |
| // screenshot() intentionally omitted until WebViewSession gains a capture | |
| // capability (a device.webview.screenshot RPC, or a native-screenshot crop to | |
| // the webview's nativeBounds). Adding it later is a non-breaking change. | |
| async screenshot(): Promise<Buffer> { | |
| throw new Error('Page.screenshot() is not yet supported for WebViewSession'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/mobilewright-core/src/page.ts` around lines 158 - 160, Preserve the
exported Page.screenshot() method as a compatibility stub: add back the
Page.screenshot method (same signature as before) that immediately throws a
clear, documented error explaining screenshots are unsupported until
WebViewSession implements a capture capability (e.g., device.webview.screenshot
RPC or native-screenshot with nativeBounds). Locate the Page class in page.ts
and reintroduce the method name Page.screenshot() so callers keep a stable API
and the runtime error references WebViewSession/capture RPC support.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/src/conformance/real-navigation.test.ts`:
- Around line 10-35: The live-site test "navigates to a live page and drives it
like Playwright" should be made opt-in: read a boolean env var (e.g.
process.env.RUN_LIVE_NAV or LIVE_SITE_TESTS) at the top of the test file and,
before calling the test block for the test that uses openWebviewPage and
PYTHAGOREAN_ARTICLE, call test.skip(!enabled, 'live-site tests disabled') or
conditionally register the test only when enabled; alternatively move this test
into a separate nightly suite. Ensure you reference the test name,
openWebviewPage, and PYTHAGOREAN_ARTICLE when making the change so the gating
applies only to this live navigation spec.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 02983fab-4a4f-464a-8449-bae106b87d8a
📒 Files selected for processing (2)
e2e/src/conformance/real-navigation.test.tspackages/mobilewright-core/src/page.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/mobilewright-core/src/page.ts
Comment on lines
+10
to
+35
| test('navigates to a live page and drives it like Playwright', async ({ device, screen }) => { | ||
| const page = await openWebviewPage({ device, screen }); | ||
|
|
||
| await page.goto(PYTHAGOREAN_ARTICLE); | ||
| await page.waitForLoadState('domcontentloaded'); | ||
|
|
||
| // Page-level state reflects the real navigation. | ||
| await expect(page).toHaveURL(/Pythagorean_theorem/); | ||
| await expect(page).toHaveTitle(/Pythagorean theorem/); | ||
|
|
||
| // The article heading resolves by id and by accessible role+name. | ||
| await expect(page.locator('#firstHeading')).toContainText('Pythagorean theorem'); | ||
| await expect(page.getByRole('heading', { name: /Pythagorean theorem/ }).first()).toBeVisible(); | ||
|
|
||
| // A real article has many links; assert a lower bound rather than an exact | ||
| // count, which would drift as the page is edited. | ||
| const linkCount = await page.getByRole('link').count(); | ||
| expect(linkCount).toBeGreaterThan(50); | ||
| await expect(page.getByRole('link').first()).toBeVisible(); | ||
|
|
||
| // Scroll the last link (near the page bottom) into view and confirm it lands | ||
| // in the viewport — a skin-independent target on a long article. | ||
| const lastLink = page.getByRole('link').last(); | ||
| await lastLink.scrollIntoViewIfNeeded(); | ||
| await expect(lastLink).toBeInViewport(); | ||
| }); |
There was a problem hiding this comment.
Make live-site navigation tests opt-in to avoid CI flakiness.
Line 13 onward depends on a third-party site and mutable content, so this can fail due to internet/geo/captcha/content drift rather than regressions in Mobilewright. Please gate this test behind an explicit env flag (or move it to a non-blocking nightly suite).
Proposed hardening diff
import { test, expect } from '`@mobilewright/test`';
import { openWebviewPage } from './harness.js';
+const RUN_LIVE_WEB = process.env.MW_RUN_LIVE_WEB === '1';
+
// A real, cross-origin HTTPS page (not a data: URL) so this exercises an actual
// network navigation plus engine re-injection on the fresh document. It covers
// the subset of the web API that stays stable against a live site; the
// deterministic matcher matrix lives in the other conformance files.
const PYTHAGOREAN_ARTICLE = 'https://en.wikipedia.org/wiki/Pythagorean_theorem';
test('navigates to a live page and drives it like Playwright', async ({ device, screen }) => {
+ test.skip(!RUN_LIVE_WEB, 'Set MW_RUN_LIVE_WEB=1 to run external live-web conformance tests');
+
const page = await openWebviewPage({ device, screen });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/src/conformance/real-navigation.test.ts` around lines 10 - 35, The
live-site test "navigates to a live page and drives it like Playwright" should
be made opt-in: read a boolean env var (e.g. process.env.RUN_LIVE_NAV or
LIVE_SITE_TESTS) at the top of the test file and, before calling the test block
for the test that uses openWebviewPage and PYTHAGOREAN_ARTICLE, call
test.skip(!enabled, 'live-site tests disabled') or conditionally register the
test only when enabled; alternatively move this test into a separate nightly
suite. Ensure you reference the test name, openWebviewPage, and
PYTHAGOREAN_ARTICLE when making the change so the gating applies only to this
live navigation spec.
gmegidish
force-pushed
the
fix-improving-webview
branch
from
7fa762f to
029ef5d
Compare
| } | ||
|
|
||
| private firstElExpr(body: string): string { | ||
| return `(() => { const el = ${this.firstEl()}; ${body} })()`; |
| } | ||
|
|
||
| private firstElExpr(body: string): string { | ||
| return `(() => { const el = ${this.firstEl()}; ${body} })()`; |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/src/conformance/conformance.test.ts`:
- Around line 9-13: The suite currently creates independent top-level test()
entries from conformanceSpecs while openWebviewPage() restarts the same app,
causing races; wrap the generated tests in a serial describe so they run one at
a time. Move the for (const spec of conformanceSpecs) { test(spec.name, ... ) }
loop inside a test.describe.serial(...) (or test.describe.configure({ mode:
'serial' })) block so that the tests using openWebviewPage and the shared
PLAYGROUND_APP lifecycle run sequentially and cannot terminate/launch the app
concurrently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8be016dc-2b74-4769-adc8-93e62d0ed8e9
📒 Files selected for processing (19)
e2e/mobilewright.config.tse2e/package.jsone2e/playwright.config.tse2e/src/conformance/conformance.pw.tse2e/src/conformance/conformance.test.tse2e/src/conformance/harness.tse2e/src/conformance/specs/actions.spec.tse2e/src/conformance/specs/assertions-state.spec.tse2e/src/conformance/specs/assertions-text.spec.tse2e/src/conformance/specs/assertions-web.spec.tse2e/src/conformance/specs/fixtures.tse2e/src/conformance/specs/index.tse2e/src/conformance/specs/locators.spec.tse2e/src/conformance/specs/real-navigation.spec.tse2e/src/conformance/specs/types.tse2e/src/ios/driver-ios.test.tspackages/mobilewright-core/src/playwright-engine.tspackages/mobilewright-core/src/web-locator-reinjection.test.tspackages/mobilewright-core/src/web-locator.ts
💤 Files with no reviewable changes (1)
- e2e/src/conformance/harness.ts
✅ Files skipped from review due to trivial changes (3)
- e2e/playwright.config.ts
- e2e/src/conformance/conformance.pw.ts
- e2e/src/conformance/specs/fixtures.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/package.json
- packages/mobilewright-core/src/web-locator.ts
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
2 participants
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.
No description provided.