Skip to content

Collect browser page errors through one watcher - #345

Merged
loganj merged 3 commits into
mainfrom
larry/one-page-error-collector
Sep 28, 2026
Merged

loganj merged 3 commits into
mainfrom
larry/one-page-error-collector

Conversation

@loganj

@loganj loganj commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

🤖

Summary

  • Browser tests on main fail about once a day in WebKit on navigation-repairs.spec.mjs:89 ("edited navigation hash … survives reload and Back"). This PR stops that failure. It also stops the WebKit failure in new-message.spec.mjs:647. Together they are 8 CI failures since Friday, including both recent red builds on main.
  • Both failures come from one defect class: each test decided for itself which browser errors count as a failure. The shared fixture had one reviewed WebKit rule, but 34 private pageerror listeners in 22 files did not share it. Now one watcher, tests/browser/page-errors.mjs, collects page errors for every browser test. A lint rule rejects pageerror listeners anywhere else, so this class cannot come back quietly.
  • The app is not changed. The navigation-repairs error is a WebKit log message, not an app bug (see below).

Why the navigation-repairs error is not an app bug

  • The failing error is Fetch API cannot load http://127.0.0.1:…/api/relay/primary/query due to access control checks. It appears 18 ms after page.reload(), while a relay request is still running. The 7 CI occurrences all happen during a reload: 6 on /query and 1 on /session.
  • Playwright's WebKit backend reports every WebKit console error as a page error. WebKit writes this console error when a reload cancels a fetch that is still in progress. The trace for the same request shows Load request cancelled.
  • If the app left the rejected promise unhandled, WebKit would report a second page error named Unhandled Promise Rejection. I checked this locally in WebKit 26.6 and Chromium. The CI trace has no such error, so the app handles the rejection.

The one watcher

  • watchPageErrors(page) keeps every page error in errors as evidence. unexplained() returns the errors that are not reviewed engine reports, and tests assert that it is empty.
  • There are two reviewed WebKit-only reports:
    1. ResizeObserver loop completed with undelivered notifications. The fixture already accepted this. It now applies to all tests.
    2. The "access control checks" log for a fetch. It is accepted only when Playwright also saw that exact URL fail as cancelled (Load request cancelled on Linux, cancelled on macOS), and only once per cancellation. If no request was cancelled, if the URL is different, if the failure is a different network error, or if the page reports an Unhandled Promise Rejection, the test still fails.
  • Chromium has no accepted reports.
  • The engine comes from the page's browser, so callers do not pass browserName.
  • Page-error text now keeps the first line of the error, for example TypeError: a: b. Before, many collectors kept only the part after Playwright's first colon, and that cut WebKit URLs in half (/127.0.0.1:…).
  • Fixture pages: app.watchPageErrors(otherPage) adds a second window to the fixture's final check. Its errors are saved in the evidence file as additionalPageErrors. membership.spec and unread.spec still assert the raw app.report.errors list is empty, so they still reject even the ResizeObserver warning.
  • Lint: tests/browser/page-errors.grit is a Biome GritQL plugin, enabled in biome.json for every file except the watcher itself. It reports .on/.once/.addListener/.prependListener with "pageerror", so pnpm lint and pnpm check fail on a new private collector.

Tests

  • Browser cases added or removed: none. The assertions in the 22 migrated files are unchanged, except that they now go through the shared rules.
  • New tests/integration/page-errors.test.mjs (node:test, no browser). It replays the exact page error from the CI trace (Linux WebKit, run for main 85d6bf8) through a stand-in page. It covers: an accepted cancellation, macOS cancellation text, a missing, different, or non-cancel failure, one log per cancellation, an unhandled rejection, Chromium, and application errors.
  • Fail then pass:
    • Without the cancellation rule, 3 of the 7 new cases fail. With it, 7/7 pass.
    • Main's CI failures are the browser-level failure evidence. Local macOS WebKit does not produce this console message (0 of 30 runs), so I could not reproduce it locally.
    • The lint rule reports main's avatar-edit.spec.mjs:13 collector and passes on this branch.
  • Local runs (macOS, Playwright 1.63.0) on the pushed head, with the full files for all 20 changed or affected browser specs (the 18 migrated ones plus navigation-repairs and membership):
    • Chromium: 115 passed.
    • WebKit: 115 passed.
    • tests/fixtures/design-system/viewer.spec.ts, the 4 tests that use the watcher, both engines: 8 passed.
    • biome check, tsc, tsc -p tsconfig.design.json: clean.
  • Not run locally: conversation.spec.mjs. It runs pnpm install for a packed consumer, and the registry was unreachable from this machine. CI runs it.
  • Setup and run time are not expected to change. I did not measure them before and after.
  • A green run does not prove that the navigation-repairs flake is gone on Linux. It proves that the rule matches the recorded failure and still rejects its unhandled cases.

Larry added 3 commits September 28, 2026 13:59
Browser tests had 34 private pageerror collectors in 22 files. They did not share the
fixture's reviewed WebKit ResizeObserver rule, so new-message failed on a
warning that the fixture accepts.

tests/browser/page-errors.mjs is now the only collector. A Biome GritQL
plugin rejects pageerror listeners anywhere else.

navigation-repairs failed on "Fetch API cannot load .../query due to access
control checks." That is not an unhandled rejection. Playwright's WebKit
backend turns WebKit console errors into page errors, and WebKit logs this
line when a reload cancels a fetch still in progress. An unhandled rejection
would be a separate "Unhandled Promise Rejection" page error, and the CI trace
has none. The watcher accepts this log only when Playwright also saw that
exact request URL cancelled, once per cancellation.

Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
custom-emoji-authoring and mocked-native-ipc landed on main with private
pageerror listeners, which the lint rule now rejects.

Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
@loganj
loganj force-pushed the larry/one-page-error-collector branch from c32d032 to aa70e71 Compare September 28, 2026 18:01
@loganj
loganj marked this pull request as ready for review September 28, 2026 20:11
@loganj
loganj requested review from a team, comp615 and wesbillman as code owners September 28, 2026 20:11

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

No actionable blockers found at aa70e710afb0020800c4356c96ad074f9aa6c07d against a7b45d346d50ca7137a4f696239fe120dc1633da. The collector migrations preserve assertion boundaries, console checks, and extra-window teardown. The broader WebKit ResizeObserver allowance is an explicit part of this PR, not an accidental omission.

  • Verified locally: 7/7 watcher tests; real primary/secondary page exceptions rejected at fixture teardown in both engines, including after the primary closes; caught versus unhandled aborted fetches in Chromium and WebKit. Raw secondary errors survive page closure. Independently replayed the original Linux trace from run 36363977274: the recorded cancellation is explained, while missing-cancellation, duplicate-error and Chromium controls still fail.
  • CI: run 36462275291 is green after a WebKit shard-3 rerun. The initial failure was the unchanged search-dialog assertion in navigation.mjs:36, not error filtering. Its merge checkout c217d7cf has the same tree as the reviewed head. Counts remain 808 functional cases plus 7 measurements.
  • Residual evidence gap, not a blocker: the current hosted fixture reports contain no cancellation access-control log. The Linux allowance is validated against the recorded event, not a newly reproduced Linux end-to-end fail/pass. This does not prove the original flake is eliminated. No native-app claim or GitHub approval is made.
Hosted before/after test-cost check

Base run 36460596940 versus the PR run above; Ubuntu 24.04, Playwright 1.63.0, two functional workers. Numbers come from job timestamps and the saved ci-timing.json summary data. Setup means job start → test-command start, not application startup. Execution uses the existing workflow's pnpm test:browser:ci --project <engine> --no-deps --shard=N/3 --reporter=list,json; measurements remain serial.

Lane Setup seconds, base → PR Command wall seconds, base → PR Summed test seconds, base → PR
Chromium 1 138 → 104 461 → 616 882 → 1194
Chromium 2 100 → 95 424 → 422 780 → 769
Chromium 3 80 → 90 480 → 495 904 → 930
WebKit 1 85 → 103 736 → 739 1432 → 1440
WebKit 2 77 → 93 454 → 583 853 → 1096
WebKit 3, PR rerun 122 → 89 737 → 505 1410 → 968
Measurements 61 → 93 173 → 180 162 → 167

The initial PR WebKit-3 attempt took 710s, with 133 passed/1 failed; the table does not erase that failure. Slowest PR functional test was the packed conversation consumer (WebKit, 41.6s); slowest file was navigation-groups (182.4s). Base maxima were nested-replies (63.0s test/256.9s file). The slowest measurement remained scroll paging, 83.5 → 83.6s. Mixed runner-level increases/decreases do not establish either a causal performance regression or performance neutrality from this single comparison.

@loganj
loganj merged commit fd45046 into main Sep 28, 2026
23 of 25 checks passed
@loganj
loganj deleted the larry/one-page-error-collector branch September 28, 2026 20:26
loganj pushed a commit that referenced this pull request Sep 28, 2026
#189 added a page error collector while #345 made watchPageErrors the
only allowed collector. Both merged, so main fails the Biome
page-errors rule.

Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants